On Tue, Mar 21, 2023 at 11:01 PM Michael Paquier <mich...@paquier.xyz> wrote:
> - # Function introduced in OpenSSL 1.0.2.
> + # Functions introduced in OpenSSL 1.0.2. LibreSSL doesn't have all of
> these.
> ['X509_get_signature_nid'],
> + ['SSL_CTX_set_cert_cb'],
>
> From what I can see, X509_get_signature_nid() is in LibreSSL, but not
> SSL_CTX_set_cert_cb(). Perhaps that's worth having two different
> comments?
I took a stab at that in v18. I diverged a bit between Meson and
Autoconf, which you may not care for.
> + <para>
> + a certificate may be sent, if the server requests one and it has
> + been provided via <literal>sslcert</literal>
> + </para>
>
> It seems to me that this description is not completely exact. The
> default is to look at ~/.postgresql/postgresql.crt, so sslcert is not
> mandatory. There could be a certificate even without sslcert set.
Reworded.
> + libpq_append_conn_error(conn, "sslcertmode value \"%s\" invalid
> when SSL support is not compiled in",
> + conn->sslcertmode);
>
> This string could be combined with the same one used for sslmode,
> saving a bit in translation effortm by making the connection parameter
> name a value of the string ("%s value \"%s\" invalid ..").
Done.
> + * figure out if a certficate was actually requested, so "require" is
> s/certficate/certificate/.
Heh, fixed. I need new glasses, clearly.
> contrib/sslinfo/ has ssl_client_cert_present(), that we could use in
> the tests to make sure that the client has actually sent a
> certificate? How about adding some of these tests to 003_sslinfo.pl
> for the "allow" and "require" cases?
Added; see what you think.
> + if (!conn->ssl_cert_requested)
> + {
> + libpq_append_conn_error(conn, "server did not request a
> certificate");
> + return false;
> + }
> + else if (!conn->ssl_cert_sent)
> + {
> + libpq_append_conn_error(conn, "server accepted connection without
> a valid certificate");
> + return false;
> + }
> Perhaps useless question: should this say "SSL certificate"?
I have no objection, so done that way.
> freePGconn() is missing a free(sslcertmode).
Argh, I keep forgetting that. Fixed, thanks!
--Jacob
1: 8d93ca3792 ! 1: ad71dc8b72 Add sslcertmode option for client certificates
@@ configure: else
fi
- # Function introduced in OpenSSL 1.0.2.
- for ac_func in X509_get_signature_nid
-+ # Functions introduced in OpenSSL 1.0.2. LibreSSL doesn't have all of
these.
++ # Functions introduced in OpenSSL 1.0.2. Note that LibreSSL doesn't have
++ # SSL_CTX_set_cert_cb().
+ for ac_func in X509_get_signature_nid SSL_CTX_set_cert_cb
do :
- ac_fn_c_check_func "$LINENO" "X509_get_signature_nid"
"ac_cv_func_X509_get_signature_nid"
@@ configure.ac: if test "$with_ssl" = openssl ; then
fi
- # Function introduced in OpenSSL 1.0.2.
- AC_CHECK_FUNCS([X509_get_signature_nid])
-+ # Functions introduced in OpenSSL 1.0.2. LibreSSL doesn't have all of
these.
++ # Functions introduced in OpenSSL 1.0.2. Note that LibreSSL doesn't have
++ # SSL_CTX_set_cert_cb().
+ AC_CHECK_FUNCS([X509_get_signature_nid SSL_CTX_set_cert_cb])
# Functions introduced in OpenSSL 1.1.0. We used to check for
# OPENSSL_VERSION_NUMBER, but that didn't work with 1.1.0, because
LibreSSL
@@ doc/src/sgml/libpq.sgml: postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
+ <term><literal>allow</literal> (default)</term>
+ <listitem>
+ <para>
-+ a certificate may be sent, if the server requests one and it
has
-+ been provided via <literal>sslcert</literal>
++ a certificate may be sent, if the server requests one and the
client
++ has one to send
+ </para>
+ </listitem>
+ </varlistentry>
@@ meson.build: if sslopt in ['auto', 'openssl']
['SSL_new', {'required': true}],
- # Function introduced in OpenSSL 1.0.2.
-+ # Functions introduced in OpenSSL 1.0.2. LibreSSL doesn't have all
of these.
++ # Functions introduced in OpenSSL 1.0.2.
['X509_get_signature_nid'],
-+ ['SSL_CTX_set_cert_cb'],
++ ['SSL_CTX_set_cert_cb'], # not in LibreSSL
# Functions introduced in OpenSSL 1.1.0. We used to check for
# OPENSSL_VERSION_NUMBER, but that didn't work with 1.1.0, because
LibreSSL
@@ src/interfaces/libpq/fe-auth.c: check_expected_areq(AuthRequest areq,
PGconn *co
+ */
+ if (!conn->ssl_cert_requested)
+ {
-+ libpq_append_conn_error(conn, "server did not request a
certificate");
++ libpq_append_conn_error(conn, "server did not request
an SSL certificate");
+ return false;
+ }
+ else if (!conn->ssl_cert_sent)
+ {
-+ libpq_append_conn_error(conn, "server accepted
connection without a valid certificate");
++ libpq_append_conn_error(conn, "server accepted
connection without a valid SSL certificate");
+ return false;
+ }
+ }
@@ src/interfaces/libpq/fe-connect.c: static const internalPQconninfoOption
PQconni
{"sslpassword", NULL, NULL, NULL,
"SSL-Client-Key-Password", "*", 20,
offsetof(struct pg_conn, sslpassword)},
+@@ src/interfaces/libpq/fe-connect.c: connectOptions2(PGconn *conn)
+ case 'r': /* "require" */
+ case 'v': /* "verify-ca" or
"verify-full" */
+ conn->status = CONNECTION_BAD;
+- libpq_append_conn_error(conn, "sslmode value
\"%s\" invalid when SSL support is not compiled in",
+-
conn->sslmode);
++ libpq_append_conn_error(conn, "%s value \"%s\"
invalid when SSL support is not compiled in",
++
"sslmode", conn->sslmode);
+ return false;
+ }
+ #endif
@@ src/interfaces/libpq/fe-connect.c: connectOptions2(PGconn *conn)
return false;
}
@@ src/interfaces/libpq/fe-connect.c: connectOptions2(PGconn *conn)
+ if (strcmp(conn->sslcertmode, "require") == 0)
+ {
+ conn->status = CONNECTION_BAD;
-+ libpq_append_conn_error(conn, "sslcertmode value \"%s\"
invalid when SSL support is not compiled in",
-+
conn->sslcertmode);
++ libpq_append_conn_error(conn, "%s value \"%s\" invalid
when SSL support is not compiled in",
++
"sslcertmode", conn->sslcertmode);
+ return false;
+ }
+#endif
+#ifndef HAVE_SSL_CTX_SET_CERT_CB
+ /*
+ * Without a certificate callback, the current implementation
can't
-+ * figure out if a certficate was actually requested, so
"require" is
++ * figure out if a certificate was actually requested, so
"require" is
+ * useless.
+ */
+ if (strcmp(conn->sslcertmode, "require") == 0)
@@ src/interfaces/libpq/fe-connect.c: connectOptions2(PGconn *conn)
/*
* validate gssencmode option
*/
+@@ src/interfaces/libpq/fe-connect.c: freePGconn(PGconn *conn)
+ explicit_bzero(conn->sslpassword, strlen(conn->sslpassword));
+ free(conn->sslpassword);
+ }
++ free(conn->sslcertmode);
+ free(conn->sslrootcert);
+ free(conn->sslcrl);
+ free(conn->sslcrldir);
## src/interfaces/libpq/fe-secure-openssl.c ##
@@ src/interfaces/libpq/fe-secure-openssl.c: verify_cb(int ok,
X509_STORE_CTX *ctx)
@@ src/test/ssl/t/001_ssltests.pl: $node->connect_ok(
+ "$common_connstr sslrootcert=ssl/root+server_ca.crt sslmode=require
sslcertmode=require",
+ "connect with sslcertmode=require fails without a client certificate",
+ expected_stderr => $supports_sslcertmode_require
-+ ? qr/server accepted connection without a valid certificate/
++ ? qr/server accepted connection without a valid SSL certificate/
+ : qr/sslcertmode value "require" is not supported/);
+
# CRL tests
@@ src/test/ssl/t/001_ssltests.pl: $node->connect_ok(
$node->connect_fails(
"$common_connstr user=ssltestuser sslcert=ssl/client.crt "
+ ## src/test/ssl/t/003_sslinfo.pl ##
+@@ src/test/ssl/t/003_sslinfo.pl: my $SERVERHOSTADDR = '127.0.0.1';
+ # This is the pattern to use in pg_hba.conf to match incoming connections.
+ my $SERVERHOSTCIDR = '127.0.0.1/32';
+
++# Determine whether build supports sslcertmode=require.
++my $supports_sslcertmode_require =
++ check_pg_config("#define HAVE_SSL_CTX_SET_CERT_CB 1");
++
+ # Allocation of base connection string shared among multiple tests.
+ my $common_connstr;
+
+@@ src/test/ssl/t/003_sslinfo.pl: $result = $node->safe_psql(
+ connstr => $common_connstr);
+ is($result, 'CA:FALSE|t', 'extract extension from cert');
+
++# Sanity tests for sslcertmode, using ssl_client_cert_present()
++my @cases = (
++ { opts => "sslcertmode=allow", present
=> 't' },
++ { opts => "sslcertmode=allow sslcert=invalid", present => 'f' },
++ { opts => "sslcertmode=disable", present
=> 'f' },
++);
++if ($supports_sslcertmode_require)
++{
++ push(@cases, { opts => "sslcertmode=require", present => 't' });
++}
++
++foreach my $c (@cases) {
++ $result = $node->safe_psql(
++ "trustdb",
++ "SELECT ssl_client_cert_present();",
++ connstr => "$common_connstr dbname=trustdb $c->{'opts'}"
++ );
++ is($result, $c->{'present'}, "ssl_client_cert_present() for
$c->{'opts'}");
++}
++
+ done_testing();
+
## src/tools/msvc/Solution.pm ##
@@ src/tools/msvc/Solution.pm: sub GenerateFiles
HAVE_SETPROCTITLE_FAST => undef,
2: e2343c0089 ! 2: c9ecdd54ea require_auth: decouple SASL and SCRAM
@@ src/interfaces/libpq/fe-connect.c: connectOptions2(PGconn *conn)
+ free(part);
+ continue; /* avoid the bitmask manipulation
below */
}
- else if (strcmp(method, "creds") == 0)
+ else if (strcmp(method, "none") == 0)
{
## src/interfaces/libpq/libpq-int.h ##
From ad71dc8b727fc57548693946904de53c2be7defc Mon Sep 17 00:00:00 2001
From: Jacob Champion <jchamp...@timescale.com>
Date: Fri, 24 Jun 2022 15:40:42 -0700
Subject: [PATCH v18 1/2] Add sslcertmode option for client certificates
The sslcertmode option controls whether the server is allowed and/or
required to request a certificate from the client. There are three
modes:
- "allow" is the default and follows the current behavior -- a
configured sslcert is sent if the server requests one (which, with
the current implementation, will happen whenever TLS is negotiated).
- "disable" causes the client to refuse to send a client certificate
even if an sslcert is configured.
- "require" causes the client to fail if a client certificate is never
sent and the server opens a connection anyway. This doesn't add any
additional security, since there is no guarantee that the server is
validating the certificate correctly, but it may help troubleshoot
more complicated TLS setups.
sslcertmode=require needs the OpenSSL implementation to support
SSL_CTX_set_cert_cb(). Notably, LibreSSL does not.
---
configure | 12 +++--
configure.ac | 5 +-
doc/src/sgml/libpq.sgml | 64 ++++++++++++++++++++++++
meson.build | 3 +-
src/include/pg_config.h.in | 3 ++
src/interfaces/libpq/fe-auth.c | 19 +++++++
src/interfaces/libpq/fe-connect.c | 56 ++++++++++++++++++++-
src/interfaces/libpq/fe-secure-openssl.c | 39 ++++++++++++++-
src/interfaces/libpq/libpq-int.h | 3 ++
src/test/ssl/t/001_ssltests.pl | 43 ++++++++++++++++
src/test/ssl/t/003_sslinfo.pl | 24 +++++++++
src/tools/msvc/Solution.pm | 9 ++++
12 files changed, 269 insertions(+), 11 deletions(-)
diff --git a/configure b/configure
index e221dd5b0f..037e8ace54 100755
--- a/configure
+++ b/configure
@@ -12973,13 +12973,15 @@ else
fi
fi
- # Function introduced in OpenSSL 1.0.2.
- for ac_func in X509_get_signature_nid
+ # Functions introduced in OpenSSL 1.0.2. Note that LibreSSL doesn't have
+ # SSL_CTX_set_cert_cb().
+ for ac_func in X509_get_signature_nid SSL_CTX_set_cert_cb
do :
- ac_fn_c_check_func "$LINENO" "X509_get_signature_nid" "ac_cv_func_X509_get_signature_nid"
-if test "x$ac_cv_func_X509_get_signature_nid" = xyes; then :
+ as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
+ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
+if eval test \"x\$"$as_ac_var"\" = x"yes"; then :
cat >>confdefs.h <<_ACEOF
-#define HAVE_X509_GET_SIGNATURE_NID 1
+#define `$as_echo "HAVE_$ac_func" | $as_tr_cpp` 1
_ACEOF
fi
diff --git a/configure.ac b/configure.ac
index 3aa6c15c13..f3ae7569d8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1373,8 +1373,9 @@ if test "$with_ssl" = openssl ; then
AC_SEARCH_LIBS(CRYPTO_new_ex_data, [eay32 crypto], [], [AC_MSG_ERROR([library 'eay32' or 'crypto' is required for OpenSSL])])
AC_SEARCH_LIBS(SSL_new, [ssleay32 ssl], [], [AC_MSG_ERROR([library 'ssleay32' or 'ssl' is required for OpenSSL])])
fi
- # Function introduced in OpenSSL 1.0.2.
- AC_CHECK_FUNCS([X509_get_signature_nid])
+ # Functions introduced in OpenSSL 1.0.2. Note that LibreSSL doesn't have
+ # SSL_CTX_set_cert_cb().
+ AC_CHECK_FUNCS([X509_get_signature_nid SSL_CTX_set_cert_cb])
# Functions introduced in OpenSSL 1.1.0. We used to check for
# OPENSSL_VERSION_NUMBER, but that didn't work with 1.1.0, because LibreSSL
# defines OPENSSL_VERSION_NUMBER to claim version 2.0.0, even though it
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 9ee5532c07..d4cc470be7 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1810,6 +1810,60 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
</listitem>
</varlistentry>
+ <varlistentry id="libpq-connect-sslcertmode" xreflabel="sslcertmode">
+ <term><literal>sslcertmode</literal></term>
+ <listitem>
+ <para>
+ This option determines whether a client certificate may be sent to the
+ server, and whether the server is required to request one. There are
+ three modes:
+
+ <variablelist>
+ <varlistentry>
+ <term><literal>disable</literal></term>
+ <listitem>
+ <para>
+ a client certificate is never sent, even if one is provided via
+ <xref linkend="libpq-connect-sslcert" />
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>allow</literal> (default)</term>
+ <listitem>
+ <para>
+ a certificate may be sent, if the server requests one and the client
+ has one to send
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>require</literal></term>
+ <listitem>
+ <para>
+ the server <emphasis>must</emphasis> request a certificate. The
+ connection will fail if the client does not send a certificate and
+ the server successfully authenticates the client anyway.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+ </para>
+
+ <note>
+ <para>
+ <literal>sslcertmode=require</literal> doesn't add any additional
+ security, since there is no guarantee that the server is validating the
+ certificate correctly; PostgreSQL servers generally request TLS
+ certificates from clients whether they validate them or not. The option
+ may be useful when troubleshooting more complicated TLS setups.
+ </para>
+ </note>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="libpq-connect-sslrootcert" xreflabel="sslrootcert">
<term><literal>sslrootcert</literal></term>
<listitem>
@@ -7986,6 +8040,16 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
</para>
</listitem>
+ <listitem>
+ <para>
+ <indexterm>
+ <primary><envar>PGSSLCERTMODE</envar></primary>
+ </indexterm>
+ <envar>PGSSLCERTMODE</envar> behaves the same as the <xref
+ linkend="libpq-connect-sslcertmode"/> connection parameter.
+ </para>
+ </listitem>
+
<listitem>
<para>
<indexterm>
diff --git a/meson.build b/meson.build
index 7f76a101ec..b0bdc17b1d 100644
--- a/meson.build
+++ b/meson.build
@@ -1219,8 +1219,9 @@ if sslopt in ['auto', 'openssl']
['CRYPTO_new_ex_data', {'required': true}],
['SSL_new', {'required': true}],
- # Function introduced in OpenSSL 1.0.2.
+ # Functions introduced in OpenSSL 1.0.2.
['X509_get_signature_nid'],
+ ['SSL_CTX_set_cert_cb'], # not in LibreSSL
# Functions introduced in OpenSSL 1.1.0. We used to check for
# OPENSSL_VERSION_NUMBER, but that didn't work with 1.1.0, because LibreSSL
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 4882c70559..3665e799e7 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -394,6 +394,9 @@
/* Define to 1 if you have spinlocks. */
#undef HAVE_SPINLOCKS
+/* Define to 1 if you have the `SSL_CTX_set_cert_cb' function. */
+#undef HAVE_SSL_CTX_SET_CERT_CB
+
/* Define to 1 if stdbool.h conforms to C99. */
#undef HAVE_STDBOOL_H
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index fa95f8e6e9..934e3f4f7c 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -798,6 +798,25 @@ check_expected_areq(AuthRequest areq, PGconn *conn)
StaticAssertDecl((sizeof(conn->allowed_auth_methods) * CHAR_BIT) > AUTH_REQ_MAX,
"AUTH_REQ_MAX overflows the allowed_auth_methods bitmask");
+ if (conn->sslcertmode[0] == 'r' /* require */
+ && areq == AUTH_REQ_OK)
+ {
+ /*
+ * Trade off a little bit of complexity to try to get these error
+ * messages as precise as possible.
+ */
+ if (!conn->ssl_cert_requested)
+ {
+ libpq_append_conn_error(conn, "server did not request an SSL certificate");
+ return false;
+ }
+ else if (!conn->ssl_cert_sent)
+ {
+ libpq_append_conn_error(conn, "server accepted connection without a valid SSL certificate");
+ return false;
+ }
+ }
+
/*
* If the user required a specific auth method, or specified an allowed
* set, then reject all others here, and make sure the server actually
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index b9f899c552..15515fbd45 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -125,8 +125,10 @@ static int ldapServiceLookup(const char *purl, PQconninfoOption *options,
#define DefaultTargetSessionAttrs "any"
#ifdef USE_SSL
#define DefaultSSLMode "prefer"
+#define DefaultSSLCertMode "allow"
#else
#define DefaultSSLMode "disable"
+#define DefaultSSLCertMode "disable"
#endif
#ifdef ENABLE_GSS
#include "fe-gssapi-common.h"
@@ -283,6 +285,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
"SSL-Client-Key", "", 64,
offsetof(struct pg_conn, sslkey)},
+ {"sslcertmode", "PGSSLCERTMODE", NULL, NULL,
+ "SSL-Client-Cert-Mode", "", 8, /* sizeof("disable") == 8 */
+ offsetof(struct pg_conn, sslcertmode)},
+
{"sslpassword", NULL, NULL, NULL,
"SSL-Client-Key-Password", "*", 20,
offsetof(struct pg_conn, sslpassword)},
@@ -1457,8 +1463,8 @@ connectOptions2(PGconn *conn)
case 'r': /* "require" */
case 'v': /* "verify-ca" or "verify-full" */
conn->status = CONNECTION_BAD;
- libpq_append_conn_error(conn, "sslmode value \"%s\" invalid when SSL support is not compiled in",
- conn->sslmode);
+ libpq_append_conn_error(conn, "%s value \"%s\" invalid when SSL support is not compiled in",
+ "sslmode", conn->sslmode);
return false;
}
#endif
@@ -1506,6 +1512,51 @@ connectOptions2(PGconn *conn)
return false;
}
+ /*
+ * validate sslcertmode option
+ */
+ if (conn->sslcertmode)
+ {
+ if (strcmp(conn->sslcertmode, "disable") != 0 &&
+ strcmp(conn->sslcertmode, "allow") != 0 &&
+ strcmp(conn->sslcertmode, "require") != 0)
+ {
+ conn->status = CONNECTION_BAD;
+ libpq_append_conn_error(conn, "invalid %s value: \"%s\"",
+ "sslcertmode", conn->sslcertmode);
+ return false;
+ }
+#ifndef USE_SSL
+ if (strcmp(conn->sslcertmode, "require") == 0)
+ {
+ conn->status = CONNECTION_BAD;
+ libpq_append_conn_error(conn, "%s value \"%s\" invalid when SSL support is not compiled in",
+ "sslcertmode", conn->sslcertmode);
+ return false;
+ }
+#endif
+#ifndef HAVE_SSL_CTX_SET_CERT_CB
+ /*
+ * Without a certificate callback, the current implementation can't
+ * figure out if a certificate was actually requested, so "require" is
+ * useless.
+ */
+ if (strcmp(conn->sslcertmode, "require") == 0)
+ {
+ conn->status = CONNECTION_BAD;
+ libpq_append_conn_error(conn, "sslcertmode value \"%s\" is not supported (check OpenSSL version)",
+ conn->sslcertmode);
+ return false;
+ }
+#endif
+ }
+ else
+ {
+ conn->sslcertmode = strdup(DefaultSSLCertMode);
+ if (!conn->sslcertmode)
+ goto oom_error;
+ }
+
/*
* validate gssencmode option
*/
@@ -4238,6 +4289,7 @@ freePGconn(PGconn *conn)
explicit_bzero(conn->sslpassword, strlen(conn->sslpassword));
free(conn->sslpassword);
}
+ free(conn->sslcertmode);
free(conn->sslrootcert);
free(conn->sslcrl);
free(conn->sslcrldir);
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 6a4431ddfe..b88d9da3e2 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -462,6 +462,33 @@ verify_cb(int ok, X509_STORE_CTX *ctx)
return ok;
}
+#ifdef HAVE_SSL_CTX_SET_CERT_CB
+/*
+ * Certificate selection callback
+ *
+ * This callback lets us choose the client certificate we send to the server
+ * after seeing its CertificateRequest. We only support sending a single
+ * hard-coded certificate via sslcert, so we don't actually set any certificates
+ * here; we just use it to record whether or not the server has actually asked
+ * for one and whether we have one to send.
+ */
+static int
+cert_cb(SSL *ssl, void *arg)
+{
+ PGconn *conn = arg;
+ conn->ssl_cert_requested = true;
+
+ /* Do we have a certificate loaded to send back? */
+ if (SSL_get_certificate(ssl))
+ conn->ssl_cert_sent = true;
+
+ /*
+ * Tell OpenSSL that the callback succeeded; we're not required to actually
+ * make any changes to the SSL handle.
+ */
+ return 1;
+}
+#endif
/*
* OpenSSL-specific wrapper around
@@ -953,6 +980,11 @@ initialize_SSL(PGconn *conn)
SSL_CTX_set_default_passwd_cb_userdata(SSL_context, conn);
}
+#ifdef HAVE_SSL_CTX_SET_CERT_CB
+ /* Set up a certificate selection callback. */
+ SSL_CTX_set_cert_cb(SSL_context, cert_cb, conn);
+#endif
+
/* Disable old protocol versions */
SSL_CTX_set_options(SSL_context, SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3);
@@ -1107,7 +1139,12 @@ initialize_SSL(PGconn *conn)
else
fnbuf[0] = '\0';
- if (fnbuf[0] == '\0')
+ if (conn->sslcertmode[0] == 'd') /* disable */
+ {
+ /* don't send a client cert even if we have one */
+ have_cert = false;
+ }
+ else if (fnbuf[0] == '\0')
{
/* no home directory, proceed without a client cert */
have_cert = false;
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 1dc264fe54..f1f1d973cc 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -384,6 +384,7 @@ struct pg_conn
char *sslkey; /* client key filename */
char *sslcert; /* client certificate filename */
char *sslpassword; /* client key file password */
+ char *sslcertmode; /* client cert mode (require,allow,disable) */
char *sslrootcert; /* root certificate filename */
char *sslcrl; /* certificate revocation list filename */
char *sslcrldir; /* certificate revocation list directory name */
@@ -527,6 +528,8 @@ struct pg_conn
/* SSL structures */
bool ssl_in_use;
+ bool ssl_cert_requested; /* Did the server ask us for a cert? */
+ bool ssl_cert_sent; /* Did we send one in reply? */
#ifdef USE_SSL
bool allow_ssl_try; /* Allowed to try SSL negotiation */
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 3094e27af3..3e5d30c37d 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -42,6 +42,10 @@ my $SERVERHOSTADDR = '127.0.0.1';
# This is the pattern to use in pg_hba.conf to match incoming connections.
my $SERVERHOSTCIDR = '127.0.0.1/32';
+# Determine whether build supports sslcertmode=require.
+my $supports_sslcertmode_require =
+ check_pg_config("#define HAVE_SSL_CTX_SET_CERT_CB 1");
+
# Allocation of base connection string shared among multiple tests.
my $common_connstr;
@@ -191,6 +195,22 @@ $node->connect_ok(
"$common_connstr sslrootcert=ssl/both-cas-2.crt sslmode=verify-ca",
"cert root file that contains two certificates, order 2");
+# sslcertmode=allow and =disable should both work without a client certificate.
+$node->connect_ok(
+ "$common_connstr sslrootcert=ssl/root+server_ca.crt sslmode=require sslcertmode=disable",
+ "connect with sslcertmode=disable");
+$node->connect_ok(
+ "$common_connstr sslrootcert=ssl/root+server_ca.crt sslmode=require sslcertmode=allow",
+ "connect with sslcertmode=allow");
+
+# sslcertmode=require, however, should fail.
+$node->connect_fails(
+ "$common_connstr sslrootcert=ssl/root+server_ca.crt sslmode=require sslcertmode=require",
+ "connect with sslcertmode=require fails without a client certificate",
+ expected_stderr => $supports_sslcertmode_require
+ ? qr/server accepted connection without a valid SSL certificate/
+ : qr/sslcertmode value "require" is not supported/);
+
# CRL tests
# Invalid CRL filename is the same as no CRL, succeeds
@@ -538,6 +558,29 @@ $node->connect_ok(
"certificate authorization succeeds with correct client cert in encrypted DER format"
);
+# correct client cert with required/allowed certificate authentication
+if ($supports_sslcertmode_require)
+{
+ $node->connect_ok(
+ "$common_connstr user=ssltestuser sslcertmode=require sslcert=ssl/client.crt "
+ . sslkey('client.key'),
+ "certificate authorization succeeds with sslcertmode=require"
+ );
+}
+$node->connect_ok(
+ "$common_connstr user=ssltestuser sslcertmode=allow sslcert=ssl/client.crt "
+ . sslkey('client.key'),
+ "certificate authorization succeeds with sslcertmode=allow"
+);
+
+# client cert isn't sent if certificate authentication is disabled
+$node->connect_fails(
+ "$common_connstr user=ssltestuser sslcertmode=disable sslcert=ssl/client.crt "
+ . sslkey('client.key'),
+ "certificate authorization fails with sslcertmode=disable",
+ expected_stderr => qr/connection requires a valid client certificate/
+);
+
# correct client cert in encrypted PEM with wrong password
$node->connect_fails(
"$common_connstr user=ssltestuser sslcert=ssl/client.crt "
diff --git a/src/test/ssl/t/003_sslinfo.pl b/src/test/ssl/t/003_sslinfo.pl
index 3f498fff70..e63256a3b8 100644
--- a/src/test/ssl/t/003_sslinfo.pl
+++ b/src/test/ssl/t/003_sslinfo.pl
@@ -43,6 +43,10 @@ my $SERVERHOSTADDR = '127.0.0.1';
# This is the pattern to use in pg_hba.conf to match incoming connections.
my $SERVERHOSTCIDR = '127.0.0.1/32';
+# Determine whether build supports sslcertmode=require.
+my $supports_sslcertmode_require =
+ check_pg_config("#define HAVE_SSL_CTX_SET_CERT_CB 1");
+
# Allocation of base connection string shared among multiple tests.
my $common_connstr;
@@ -166,4 +170,24 @@ $result = $node->safe_psql(
connstr => $common_connstr);
is($result, 'CA:FALSE|t', 'extract extension from cert');
+# Sanity tests for sslcertmode, using ssl_client_cert_present()
+my @cases = (
+ { opts => "sslcertmode=allow", present => 't' },
+ { opts => "sslcertmode=allow sslcert=invalid", present => 'f' },
+ { opts => "sslcertmode=disable", present => 'f' },
+);
+if ($supports_sslcertmode_require)
+{
+ push(@cases, { opts => "sslcertmode=require", present => 't' });
+}
+
+foreach my $c (@cases) {
+ $result = $node->safe_psql(
+ "trustdb",
+ "SELECT ssl_client_cert_present();",
+ connstr => "$common_connstr dbname=trustdb $c->{'opts'}"
+ );
+ is($result, $c->{'present'}, "ssl_client_cert_present() for $c->{'opts'}");
+}
+
done_testing();
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index b59953e5b5..153be7be11 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -327,6 +327,7 @@ sub GenerateFiles
HAVE_SETPROCTITLE_FAST => undef,
HAVE_SOCKLEN_T => 1,
HAVE_SPINLOCKS => 1,
+ HAVE_SSL_CTX_SET_CERT_CB => undef,
HAVE_STDBOOL_H => 1,
HAVE_STDINT_H => 1,
HAVE_STDLIB_H => 1,
@@ -506,6 +507,14 @@ sub GenerateFiles
$define{HAVE_HMAC_CTX_NEW} = 1;
$define{HAVE_OPENSSL_INIT_SSL} = 1;
}
+
+ # Symbols needed with OpenSSL 1.0.2 and above.
+ if ( ($digit1 >= '3' && $digit2 >= '0' && $digit3 >= '0')
+ || ($digit1 >= '1' && $digit2 >= '1' && $digit3 >= '0')
+ || ($digit1 >= '1' && $digit2 >= '0' && $digit3 >= '2'))
+ {
+ $define{HAVE_SSL_CTX_SET_CERT_CB} = 1;
+ }
}
$self->GenerateConfigHeader('src/include/pg_config.h', \%define, 1);
--
2.25.1
From c9ecdd54eaa7a9dd9e9eb34ea1fbafcd22a7c2bc Mon Sep 17 00:00:00 2001
From: Jacob Champion <jchamp...@timescale.com>
Date: Tue, 18 Oct 2022 16:55:36 -0700
Subject: [PATCH v18 2/2] require_auth: decouple SASL and SCRAM
Rather than assume that an AUTH_REQ_SASL* code refers to SCRAM-SHA-256,
future-proof by separating the single allowlist into a list of allowed
authentication request codes and a list of allowed SASL mechanisms.
The require_auth check is now separated into two tiers. The
AUTH_REQ_SASL* codes are always allowed. If the server sends one,
responsibility for the check then falls to pg_SASL_init(), which
compares the selected mechanism against the list of allowed mechanisms.
(Other SCRAM code is already responsible for rejecting unexpected or
out-of-order AUTH_REQ_SASL_* codes, so that's not explicitly handled
with this addition.)
Since there's only one recognized SASL mechanism, conn->sasl_mechs
currently only points at static hardcoded lists. Whenever a second
mechanism is added, the list will need to be managed dynamically.
---
src/interfaces/libpq/fe-auth.c | 34 +++++++++++++++++++
src/interfaces/libpq/fe-connect.c | 41 +++++++++++++++++++----
src/interfaces/libpq/libpq-int.h | 3 +-
src/test/authentication/t/001_password.pl | 14 +++++---
src/test/ssl/t/002_scram.pl | 6 ++++
5 files changed, 86 insertions(+), 12 deletions(-)
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 934e3f4f7c..7ce0e16b18 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -522,6 +522,40 @@ pg_SASL_init(PGconn *conn, int payloadlen)
goto error;
}
+ /*
+ * Before going ahead with any SASL exchange, ensure that the user has
+ * allowed (or, alternatively, has not forbidden) this particular mechanism.
+ *
+ * In a hypothetical future where a server responds with multiple SASL
+ * mechanism families, we would need to instead consult this list up above,
+ * during mechanism negotiation. We don't live in that world yet. The server
+ * presents one auth method and we decide whether that's acceptable or not.
+ */
+ if (conn->sasl_mechs)
+ {
+ const char **mech;
+ bool found = false;
+
+ Assert(conn->require_auth);
+
+ for (mech = conn->sasl_mechs; *mech; mech++)
+ {
+ if (strcmp(*mech, selected_mechanism) == 0)
+ {
+ found = true;
+ break;
+ }
+ }
+
+ if ((conn->sasl_mechs_denied && found)
+ || (!conn->sasl_mechs_denied && !found))
+ {
+ libpq_append_conn_error(conn, "auth method \"%s\" requirement failed: server requested unacceptable SASL mechanism \"%s\"",
+ conn->require_auth, selected_mechanism);
+ goto error;
+ }
+ }
+
if (conn->channel_binding[0] == 'r' && /* require */
strcmp(selected_mechanism, SCRAM_SHA_256_PLUS_NAME) != 0)
{
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 15515fbd45..c90a23fd2a 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1258,12 +1258,25 @@ connectOptions2(PGconn *conn)
more;
bool negated = false;
+ static const uint32 default_methods = (
+ 1 << AUTH_REQ_SASL
+ | 1 << AUTH_REQ_SASL_CONT
+ | 1 << AUTH_REQ_SASL_FIN
+ );
+ static const char *no_mechs[] = { NULL };
+
/*
- * By default, start from an empty set of allowed options and add to
+ * By default, start from a minimum set of allowed options and add to
* it.
+ *
+ * NB: The SASL method codes are always "allowed" here. If the server
+ * requests SASL auth, pg_SASL_init() will enforce adherence to the
+ * sasl_mechs list, which by default is empty.
*/
conn->auth_required = true;
- conn->allowed_auth_methods = 0;
+ conn->allowed_auth_methods = default_methods;
+ conn->sasl_mechs = no_mechs;
+ conn->sasl_mechs_denied = false;
for (first = true, more = true; more; first = false)
{
@@ -1290,6 +1303,9 @@ connectOptions2(PGconn *conn)
*/
conn->auth_required = false;
conn->allowed_auth_methods = -1;
+
+ /* conn->sasl_mechs is now a list of denied mechanisms. */
+ conn->sasl_mechs_denied = true;
}
else if (!negated)
{
@@ -1334,10 +1350,23 @@ connectOptions2(PGconn *conn)
}
else if (strcmp(method, "scram-sha-256") == 0)
{
- /* This currently assumes that SCRAM is the only SASL method. */
- bits = (1 << AUTH_REQ_SASL);
- bits |= (1 << AUTH_REQ_SASL_CONT);
- bits |= (1 << AUTH_REQ_SASL_FIN);
+ static const char *scram_mechs[] = {
+ SCRAM_SHA_256_NAME,
+ SCRAM_SHA_256_PLUS_NAME,
+ NULL /* list terminator */
+ };
+
+ /*
+ * This currently assumes that SCRAM is the only SASL method.
+ * Once a second mechanism is added, this code will need to add
+ * to the list instead of replacing it wholesale.
+ */
+ if (conn->sasl_mechs[0])
+ goto duplicate;
+ conn->sasl_mechs = scram_mechs;
+
+ free(part);
+ continue; /* avoid the bitmask manipulation below */
}
else if (strcmp(method, "none") == 0)
{
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index f1f1d973cc..ab26292586 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -465,7 +465,8 @@ struct pg_conn
* codes */
bool client_finished_auth; /* have we finished our half of the
* authentication exchange? */
-
+ const char **sasl_mechs; /* list of allowed/denied SASL mechanisms */
+ bool sasl_mechs_denied; /* is the sasl_mechs list forbidden? */
/* Transient state needed while establishing connection */
PGTargetServerType target_server_type; /* desired session properties */
diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
index cba5d7d648..015532893c 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -301,30 +301,34 @@ $node->connect_fails(
"user=scram_role require_auth=password",
"password authentication required, fails with SCRAM auth",
expected_stderr =>
- qr/auth method "password" requirement failed: server requested SASL authentication/
+ qr/server requested unacceptable SASL mechanism "SCRAM-SHA-256"/
);
$node->connect_fails(
"user=scram_role require_auth=md5",
"md5 authentication required, fails with SCRAM auth",
expected_stderr =>
- qr/auth method "md5" requirement failed: server requested SASL authentication/
+ qr/server requested unacceptable SASL mechanism "SCRAM-SHA-256"/
);
$node->connect_fails(
"user=scram_role require_auth=none",
"all authentication forbidden, fails with SCRAM auth",
expected_stderr =>
- qr/auth method "none" requirement failed: server requested SASL authentication/
+ qr/server requested unacceptable SASL mechanism "SCRAM-SHA-256"/
);
# Authentication fails if SCRAM authentication is forbidden.
$node->connect_fails(
"user=scram_role require_auth=!scram-sha-256",
"SCRAM authentication forbidden, fails with SCRAM auth",
- expected_stderr => qr/server requested SASL authentication/);
+ expected_stderr =>
+ qr/server requested unacceptable SASL mechanism "SCRAM-SHA-256"/
+);
$node->connect_fails(
"user=scram_role require_auth=!password,!md5,!scram-sha-256",
"multiple authentication types forbidden, fails with SCRAM auth",
- expected_stderr => qr/server requested SASL authentication/);
+ expected_stderr =>
+ qr/server requested unacceptable SASL mechanism "SCRAM-SHA-256"/
+);
# Test that bad passwords are rejected.
$ENV{"PGPASSWORD"} = 'badpass';
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index 8038135697..173ac8d86b 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -157,6 +157,12 @@ if ($supports_tls_server_end_point)
"$common_connstr user=ssltestuser channel_binding=require require_auth=scram-sha-256",
"SCRAM with SSL, channel_binding=require, and require_auth=scram-sha-256"
);
+ $node->connect_fails(
+ "$common_connstr user=ssltestuser channel_binding=require require_auth=password",
+ "SCRAM with SSL, channel_binding=require, and require_auth=password",
+ expected_stderr =>
+ qr/server requested unacceptable SASL mechanism "SCRAM-SHA-256-PLUS"/
+ );
}
else
{
--
2.25.1
