On Mon, Jan 9, 2023 at 7:07 AM Jelte Fennema <postg...@jeltef.nl> wrote:
> I also took a closer look at the code, and the only comment I have is:
>
> > appendPQExpBuffer(&conn->errorMessage,
>
> These calls can all be replaced by the recently added libpq_append_conn_error
Argh, thanks for the catch. Fixed.
> Finally I tested this against a Postgres server I created on Azure and
> the new value works as expected. The only thing that I think would be
> good to change is the error message when sslmode=verify-full, and
> sslrootcert is not provided, but ~/.postgresql/root.crt is also not available.
> I think it would be good for the error to mention sslrootcert=system
Good idea. The wording I chose in v6 is
Either provide the file, use the system's trusted roots with
sslrootcert=system, or change sslmode to disable server certificate
verification.
What do you think?
Thanks!
--Jacob
1: 7618037c86 ! 1: e7e2d43b18 libpq: add sslrootcert=system to use default CAs
@@ src/interfaces/libpq/fe-secure-openssl.c: initialize_SSL(PGconn *conn)
+ {
+ char *err = SSLerrmessage(ERR_get_error());
+
-+ appendPQExpBuffer(&conn->errorMessage,
-+ libpq_gettext("could
not load system root certificate paths: %s\n"),
-+ err);
++ libpq_append_conn_error(conn, "could not load system
root certificate paths: %s",
++ err);
+ SSLerrfree(err);
+ SSL_CTX_free(SSL_context);
+ return -1;
@@ src/interfaces/libpq/fe-secure-openssl.c: initialize_SSL(PGconn *conn)
{
X509_STORE *cvstore;
+@@ src/interfaces/libpq/fe-secure-openssl.c: initialize_SSL(PGconn *conn)
+ */
+ if (fnbuf[0] == '\0')
+ libpq_append_conn_error(conn, "could not get
home directory to locate root certificate file\n"
+- "Either
provide the file or change sslmode to disable server certificate
verification.");
++ "Either
provide the file, use the system's trusted roots with sslrootcert=system, or
change sslmode to disable server certificate verification.");
+ else
+ libpq_append_conn_error(conn, "root certificate
file \"%s\" does not exist\n"
+- "Either
provide the file or change sslmode to disable server certificate
verification.", fnbuf);
++ "Either
provide the file, use the system's trusted roots with sslrootcert=system, or
change sslmode to disable server certificate verification.", fnbuf);
+ SSL_CTX_free(SSL_context);
+ return -1;
+ }
## src/test/ssl/ssl/server-cn-only+server_ca.crt (new) ##
@@
2: c392e1796e ! 2: e4d9731e1e libpq: force sslmode=verify-full for system CAs
@@ src/interfaces/libpq/fe-connect.c: connectOptions2(PGconn *conn)
+ && strcmp(conn->sslrootcert, "system") == 0)
+ {
+ conn->status = CONNECTION_BAD;
-+ appendPQExpBuffer(&conn->errorMessage,
-+ libpq_gettext("sslrootcert
value \"%s\" invalid when SSL support is not compiled in\n"),
-+ conn->sslrootcert);
++ libpq_append_conn_error(conn, "sslrootcert value \"%s\" invalid
when SSL support is not compiled in",
++
conn->sslrootcert);
+ return false;
+ }
+#endif
@@ src/interfaces/libpq/fe-connect.c: connectOptions2(PGconn *conn)
+ && strcmp(conn->sslmode, "verify-full") != 0)
+ {
+ conn->status = CONNECTION_BAD;
-+ appendPQExpBuffer(&conn->errorMessage,
-+ libpq_gettext("weak sslmode
\"%s\" may not be used with sslrootcert=system\n"),
-+ conn->sslmode);
++ libpq_append_conn_error(conn, "weak sslmode \"%s\" may not be
used with sslrootcert=system",
++ conn->sslmode);
+ return false;
+ }
+#endif
From e7e2d43b186a32a414783baa2ca55d88a5d7fe9e Mon Sep 17 00:00:00 2001
From: Jacob Champion <jchamp...@timescale.com>
Date: Mon, 24 Oct 2022 15:30:25 -0700
Subject: [PATCH v6 1/2] libpq: add sslrootcert=system to use default CAs
Based on a patch by Thomas Habets.
Note the workaround in .cirrus.yml for a strange interaction between
brew and the openssl@3 formula; hopefully this can be removed in the
near future.
Discussion: https://www.postgresql.org/message-id/flat/CA%2BkHd%2BcJwCUxVb-Gj_0ptr3_KZPwi3%2B67vK6HnLFBK9MzuYrLA%40mail.gmail.com
---
.cirrus.yml | 14 +-
configure | 289 +++++++++---------
configure.ac | 2 +
doc/src/sgml/libpq.sgml | 17 ++
doc/src/sgml/runtime.sgml | 6 +-
meson.build | 7 +
src/include/pg_config.h.in | 4 +
src/interfaces/libpq/fe-secure-openssl.c | 29 +-
src/test/ssl/ssl/server-cn-only+server_ca.crt | 38 +++
src/test/ssl/sslfiles.mk | 6 +-
src/test/ssl/t/001_ssltests.pl | 29 ++
11 files changed, 296 insertions(+), 145 deletions(-)
create mode 100644 src/test/ssl/ssl/server-cn-only+server_ca.crt
diff --git a/.cirrus.yml b/.cirrus.yml
index 69837bcd5a..5fb8a8e42f 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -464,12 +464,24 @@ task:
make \
meson \
openldap \
- openssl \
+ openssl@3 \
python \
tcl-tk \
zstd
brew cleanup -s # to reduce cache size
+
+ # brew cleanup removes the empty certs directory in OPENSSLDIR, causing
+ # OpenSSL to report unexpected errors ("unregistered scheme") during
+ # verification failures. Put it back for now as a workaround.
+ #
+ # https://github.com/orgs/Homebrew/discussions/4030
+ #
+ # Note that $(brew --prefix openssl) will give us the opt/ prefix but not
+ # the etc/ prefix, so we hardcode the full path here. openssl@3 is pinned
+ # above to try to minimize the chances of this changing beneath us, but it's
+ # brittle...
+ mkdir -p "/opt/homebrew/etc/openssl@3/certs"
upload_caches: homebrew
ccache_cache:
diff --git a/configure b/configure
index 5d07fd0bb9..8a94a109bf 100755
--- a/configure
+++ b/configure
@@ -2094,6 +2094,56 @@ $as_echo "$ac_res" >&6; }
} # ac_fn_c_check_func
+# ac_fn_c_check_decl LINENO SYMBOL VAR INCLUDES
+# ---------------------------------------------
+# Tests whether SYMBOL is declared in INCLUDES, setting cache variable VAR
+# accordingly.
+ac_fn_c_check_decl ()
+{
+ as_lineno=${as_lineno-"$1"} as_lineno_stack=as_lineno_stack=$as_lineno_stack
+ # Initialize each $ac_[]_AC_LANG_ABBREV[]_decl_warn_flag once.
+ as_decl_name=`echo $2|sed 's/ *(.*//'`
+ as_decl_use=`echo $2|sed -e 's/(/((/' -e 's/)/) 0&/' -e 's/,/) 0& (/g'`
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether $as_decl_name is declared" >&5
+$as_echo_n "checking whether $as_decl_name is declared... " >&6; }
+if eval \${$3+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ ac_save_werror_flag=$ac_c_werror_flag
+ ac_c_werror_flag="$ac_c_decl_warn_flag$ac_c_werror_flag"
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+$4
+int
+main ()
+{
+#ifndef $as_decl_name
+#ifdef __cplusplus
+ (void) $as_decl_use;
+#else
+ (void) $as_decl_name;
+#endif
+#endif
+
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+ eval "$3=yes"
+else
+ eval "$3=no"
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ ac_c_werror_flag=$ac_save_werror_flag
+fi
+eval ac_res=\$$3
+ { $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_res" >&5
+$as_echo "$ac_res" >&6; }
+ eval $as_lineno_stack; ${as_lineno_stack:+:} unset as_lineno
+
+} # ac_fn_c_check_decl
+
# ac_fn_c_check_type LINENO TYPE VAR INCLUDES
# -------------------------------------------
# Tests whether TYPE exists after having included INCLUDES, setting cache
@@ -2387,56 +2437,6 @@ rm -f conftest.val
as_fn_set_status $ac_retval
} # ac_fn_c_compute_int
-
-# ac_fn_c_check_decl LINENO SYMBOL VAR INCLUDES
-# ---------------------------------------------
-# Tests whether SYMBOL is declared in INCLUDES, setting cache variable VAR
-# accordingly.
-ac_fn_c_check_decl ()
-{
- as_lineno=${as_lineno-"$1"} as_lineno_stack=as_lineno_stack=$as_lineno_stack
- # Initialize each $ac_[]_AC_LANG_ABBREV[]_decl_warn_flag once.
- as_decl_name=`echo $2|sed 's/ *(.*//'`
- as_decl_use=`echo $2|sed -e 's/(/((/' -e 's/)/) 0&/' -e 's/,/) 0& (/g'`
- { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether $as_decl_name is declared" >&5
-$as_echo_n "checking whether $as_decl_name is declared... " >&6; }
-if eval \${$3+:} false; then :
- $as_echo_n "(cached) " >&6
-else
- ac_save_werror_flag=$ac_c_werror_flag
- ac_c_werror_flag="$ac_c_decl_warn_flag$ac_c_werror_flag"
- cat confdefs.h - <<_ACEOF >conftest.$ac_ext
-/* end confdefs.h. */
-$4
-int
-main ()
-{
-#ifndef $as_decl_name
-#ifdef __cplusplus
- (void) $as_decl_use;
-#else
- (void) $as_decl_name;
-#endif
-#endif
-
- ;
- return 0;
-}
-_ACEOF
-if ac_fn_c_try_compile "$LINENO"; then :
- eval "$3=yes"
-else
- eval "$3=no"
-fi
-rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
- ac_c_werror_flag=$ac_save_werror_flag
-fi
-eval ac_res=\$$3
- { $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_res" >&5
-$as_echo "$ac_res" >&6; }
- eval $as_lineno_stack; ${as_lineno_stack:+:} unset as_lineno
-
-} # ac_fn_c_check_decl
cat >config.log <<_ACEOF
This file contains any messages produced by compilers while
running configure, to aid debugging if configure makes a mistake.
@@ -13016,6 +13016,107 @@ _ACEOF
fi
done
+ # Let tests differentiate between vanilla OpenSSL and LibreSSL.
+ # The Clang compiler raises a warning for an undeclared identifier that matches
+# a compiler builtin function. All extant Clang versions are affected, as of
+# Clang 3.6.0. Test a builtin known to every version. This problem affects the
+# C and Objective C languages, but Clang does report an error under C++ and
+# Objective C++.
+#
+# Passing -fno-builtin to the compiler would suppress this problem. That
+# strategy would have the advantage of being insensitive to stray warnings, but
+# it would make tests less realistic.
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking how $CC reports undeclared, standard C functions" >&5
+$as_echo_n "checking how $CC reports undeclared, standard C functions... " >&6; }
+if ${ac_cv_c_decl_report+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+
+int
+main ()
+{
+(void) strchr;
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+ if test -s conftest.err; then :
+ # For AC_CHECK_DECL to react to warnings, the compiler must be silent on
+ # valid AC_CHECK_DECL input. No library function is consistently available
+ # on freestanding implementations, so test against a dummy declaration.
+ # Include always-available headers on the off chance that they somehow
+ # elicit warnings.
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include <float.h>
+#include <limits.h>
+#include <stdarg.h>
+#include <stddef.h>
+extern void ac_decl (int, char *);
+int
+main ()
+{
+#ifdef __cplusplus
+ (void) ac_decl ((int) 0, (char *) 0);
+ (void) ac_decl;
+#else
+ (void) ac_decl;
+#endif
+
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+ if test -s conftest.err; then :
+ { { $as_echo "$as_me:${as_lineno-$LINENO}: error: in \`$ac_pwd':" >&5
+$as_echo "$as_me: error: in \`$ac_pwd':" >&2;}
+as_fn_error $? "cannot detect from compiler exit status or warnings
+See \`config.log' for more details" "$LINENO" 5; }
+else
+ ac_cv_c_decl_report=warning
+fi
+else
+ { { $as_echo "$as_me:${as_lineno-$LINENO}: error: in \`$ac_pwd':" >&5
+$as_echo "$as_me: error: in \`$ac_pwd':" >&2;}
+as_fn_error $? "cannot compile a simple declaration test
+See \`config.log' for more details" "$LINENO" 5; }
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+else
+ { { $as_echo "$as_me:${as_lineno-$LINENO}: error: in \`$ac_pwd':" >&5
+$as_echo "$as_me: error: in \`$ac_pwd':" >&2;}
+as_fn_error $? "compiler does not report undeclared identifiers
+See \`config.log' for more details" "$LINENO" 5; }
+fi
+else
+ ac_cv_c_decl_report=error
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_c_decl_report" >&5
+$as_echo "$ac_cv_c_decl_report" >&6; }
+
+case $ac_cv_c_decl_report in
+ warning) ac_c_decl_warn_flag=yes ;;
+ *) ac_c_decl_warn_flag= ;;
+esac
+
+ac_fn_c_check_decl "$LINENO" "LIBRESSL_VERSION_NUMBER" "ac_cv_have_decl_LIBRESSL_VERSION_NUMBER" "#include <openssl/opensslv.h>
+"
+if test "x$ac_cv_have_decl_LIBRESSL_VERSION_NUMBER" = xyes; then :
+ ac_have_decl=1
+else
+ ac_have_decl=0
+fi
+
+cat >>confdefs.h <<_ACEOF
+#define HAVE_DECL_LIBRESSL_VERSION_NUMBER $ac_have_decl
+_ACEOF
+
$as_echo "#define USE_OPENSSL 1" >>confdefs.h
@@ -16045,94 +16146,6 @@ fi
# posix_fadvise() is a no-op on Solaris, so don't incur function overhead
# by calling it, 2009-04-02
# http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/lib/libc/port/gen/posix_fadvise.c
-# The Clang compiler raises a warning for an undeclared identifier that matches
-# a compiler builtin function. All extant Clang versions are affected, as of
-# Clang 3.6.0. Test a builtin known to every version. This problem affects the
-# C and Objective C languages, but Clang does report an error under C++ and
-# Objective C++.
-#
-# Passing -fno-builtin to the compiler would suppress this problem. That
-# strategy would have the advantage of being insensitive to stray warnings, but
-# it would make tests less realistic.
-{ $as_echo "$as_me:${as_lineno-$LINENO}: checking how $CC reports undeclared, standard C functions" >&5
-$as_echo_n "checking how $CC reports undeclared, standard C functions... " >&6; }
-if ${ac_cv_c_decl_report+:} false; then :
- $as_echo_n "(cached) " >&6
-else
- cat confdefs.h - <<_ACEOF >conftest.$ac_ext
-/* end confdefs.h. */
-
-int
-main ()
-{
-(void) strchr;
- ;
- return 0;
-}
-_ACEOF
-if ac_fn_c_try_compile "$LINENO"; then :
- if test -s conftest.err; then :
- # For AC_CHECK_DECL to react to warnings, the compiler must be silent on
- # valid AC_CHECK_DECL input. No library function is consistently available
- # on freestanding implementations, so test against a dummy declaration.
- # Include always-available headers on the off chance that they somehow
- # elicit warnings.
- cat confdefs.h - <<_ACEOF >conftest.$ac_ext
-/* end confdefs.h. */
-#include <float.h>
-#include <limits.h>
-#include <stdarg.h>
-#include <stddef.h>
-extern void ac_decl (int, char *);
-int
-main ()
-{
-#ifdef __cplusplus
- (void) ac_decl ((int) 0, (char *) 0);
- (void) ac_decl;
-#else
- (void) ac_decl;
-#endif
-
- ;
- return 0;
-}
-_ACEOF
-if ac_fn_c_try_compile "$LINENO"; then :
- if test -s conftest.err; then :
- { { $as_echo "$as_me:${as_lineno-$LINENO}: error: in \`$ac_pwd':" >&5
-$as_echo "$as_me: error: in \`$ac_pwd':" >&2;}
-as_fn_error $? "cannot detect from compiler exit status or warnings
-See \`config.log' for more details" "$LINENO" 5; }
-else
- ac_cv_c_decl_report=warning
-fi
-else
- { { $as_echo "$as_me:${as_lineno-$LINENO}: error: in \`$ac_pwd':" >&5
-$as_echo "$as_me: error: in \`$ac_pwd':" >&2;}
-as_fn_error $? "cannot compile a simple declaration test
-See \`config.log' for more details" "$LINENO" 5; }
-fi
-rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
-else
- { { $as_echo "$as_me:${as_lineno-$LINENO}: error: in \`$ac_pwd':" >&5
-$as_echo "$as_me: error: in \`$ac_pwd':" >&2;}
-as_fn_error $? "compiler does not report undeclared identifiers
-See \`config.log' for more details" "$LINENO" 5; }
-fi
-else
- ac_cv_c_decl_report=error
-fi
-rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
-fi
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_c_decl_report" >&5
-$as_echo "$ac_cv_c_decl_report" >&6; }
-
-case $ac_cv_c_decl_report in
- warning) ac_c_decl_warn_flag=yes ;;
- *) ac_c_decl_warn_flag= ;;
-esac
-
if test "$PORTNAME" != "solaris"; then :
for ac_func in posix_fadvise
diff --git a/configure.ac b/configure.ac
index e9b74ced6c..7d5289df6e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1385,6 +1385,8 @@ if test "$with_ssl" = openssl ; then
# thread-safety. In 1.1.0, it's no longer required, and CRYPTO_lock()
# function was removed.
AC_CHECK_FUNCS([CRYPTO_lock])
+ # Let tests differentiate between vanilla OpenSSL and LibreSSL.
+ AC_CHECK_DECLS([LIBRESSL_VERSION_NUMBER], [], [], [#include <openssl/opensslv.h>])
AC_DEFINE([USE_OPENSSL], 1, [Define to 1 to build with OpenSSL support. (--with-ssl=openssl)])
elif test "$with_ssl" != no ; then
AC_MSG_ERROR([--with-ssl must specify openssl])
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 0e7ae70c70..a293431020 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 9e87e07dfe..4cd3fc268b 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/meson.build b/meson.build
index 45fb9dd616..4ee3ce51e5 100644
--- a/meson.build
+++ b/meson.build
@@ -1234,6 +1234,13 @@ if get_option('ssl') == 'openssl'
endif
endforeach
+ # Let tests differentiate between vanilla OpenSSL and LibreSSL.
+ sym = 'LIBRESSL_VERSION_NUMBER'
+ found = cc.has_header_symbol('openssl/opensslv.h', sym, dependencies: ssl_int)
+ cdata.set10('HAVE_DECL_' + sym, found, description:
+'''Define to 1 if you have the declaration of `@0@', and to 0 if you
+ don't.'''.format(sym))
+
cdata.set('USE_OPENSSL', 1,
description: 'Define to 1 to build with OpenSSL support. (-Dssl=openssl)')
cdata.set('OPENSSL_API_COMPAT', '0x10001000L',
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index c5a80b829e..34055c0639 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -99,6 +99,10 @@
don't. */
#undef HAVE_DECL_F_FULLFSYNC
+/* Define to 1 if you have the declaration of `LIBRESSL_VERSION_NUMBER', and
+ to 0 if you don't. */
+#undef HAVE_DECL_LIBRESSL_VERSION_NUMBER
+
/* Define to 1 if you have the declaration of
`LLVMCreateGDBRegistrationListener', and to 0 if you don't. */
#undef HAVE_DECL_LLVMCREATEGDBREGISTRATIONLISTENER
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 983536de25..4369844a36 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1022,8 +1022,29 @@ 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());
+
+ libpq_append_conn_error(conn, "could not load system root certificate paths: %s",
+ 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;
@@ -1084,10 +1105,10 @@ initialize_SSL(PGconn *conn)
*/
if (fnbuf[0] == '\0')
libpq_append_conn_error(conn, "could not get home directory to locate root certificate file\n"
- "Either provide the file or change sslmode to disable server certificate verification.");
+ "Either provide the file, use the system's trusted roots with sslrootcert=system, or change sslmode to disable server certificate verification.");
else
libpq_append_conn_error(conn, "root certificate file \"%s\" does not exist\n"
- "Either provide the file or change sslmode to disable server certificate verification.", fnbuf);
+ "Either provide the file, use the system's trusted roots with sslrootcert=system, or change sslmode to disable server certificate verification.", fnbuf);
SSL_CTX_free(SSL_context);
return -1;
}
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 5d9dc09a4b..3cb5a28ec6 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 3094e27af3..6051ee226d 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -33,6 +33,10 @@ sub switch_server_cert
{
$ssl_server->switch_server_cert(@_);
}
+
+# Determine whether this build uses OpenSSL or LibreSSL.
+my $libressl = check_pg_config("#define HAVE_DECL_LIBRESSL_VERSION_NUMBER 1");
+
#### Some configuration
# This is the hostname used to connect to the server. This cannot be a
@@ -441,6 +445,31 @@ $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/);
+
+SKIP:
+{
+ skip "SSL_CERT_FILE is not supported with LibreSSL" if $libressl;
+
+ # 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
From e4d9731e1e13607707b21ea0c99bb2eafad11b3b Mon Sep 17 00:00:00 2001
From: Jacob Champion <jchamp...@timescale.com>
Date: Mon, 24 Oct 2022 15:24:11 -0700
Subject: [PATCH v6 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 a293431020..364a82adfd 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1733,15 +1733,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 4cd3fc268b..4de9909292 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2008,9 +2008,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 50b5df3490..69826eaadf 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1259,6 +1259,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
*/
@@ -1305,6 +1321,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.
@@ -5806,6 +5837,7 @@ static bool
conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
{
PQconninfoOption *option;
+ PQconninfoOption *sslmode_default = NULL, *sslrootcert = NULL;
char *tmp;
/*
@@ -5822,6 +5854,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 */
@@ -5864,6 +5899,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;
}
/*
@@ -5896,6 +5938,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
