From def670a45b7963f1f537b83c264937e14c279fd6 Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Fri, 24 Jun 2022 15:40:42 -0700 Subject: [PATCH v10 2/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 | 11 ++--- configure.ac | 4 +- doc/src/sgml/libpq.sgml | 54 +++++++++++++++++++++++ meson.build | 3 +- src/include/pg_config.h.in | 3 ++ src/interfaces/libpq/fe-auth.c | 21 +++++++++ src/interfaces/libpq/fe-connect.c | 55 ++++++++++++++++++++++++ src/interfaces/libpq/fe-secure-openssl.c | 39 ++++++++++++++++- src/interfaces/libpq/libpq-int.h | 3 ++ src/test/ssl/t/001_ssltests.pl | 43 ++++++++++++++++++ src/tools/msvc/Solution.pm | 8 ++++ 11 files changed, 235 insertions(+), 9 deletions(-) diff --git a/configure b/configure index e04ee9fb41..d46896e44e 100755 --- a/configure +++ b/configure @@ -12902,13 +12902,14 @@ 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. LibreSSL doesn't have all of these. + 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 f146c8301a..8458e3793d 100644 --- a/configure.ac +++ b/configure.ac @@ -1351,8 +1351,8 @@ 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. LibreSSL doesn't have all of these. + 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 05d9645f40..c06b0718cf 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -1810,6 +1810,50 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname + + sslcertmode + + + 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: + + + + disable + + + a client certificate is never sent, even if one is provided via + + + + + + + allow (default) + + + a certificate may be sent, if the server requests one and it has + been provided via sslcert + + + + + + require + + + the server must request a certificate. The + connection will fail if the server authenticates the client despite + not requesting or receiving one. + + + + + + + + sslrootcert @@ -7985,6 +8029,16 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough) + + + + PGSSLCERTMODE + + PGSSLCERTMODE behaves the same as the connection parameter. + + + diff --git a/meson.build b/meson.build index 925db70c9d..bad035c8e3 100644 --- a/meson.build +++ b/meson.build @@ -1173,8 +1173,9 @@ if get_option('ssl') == '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. LibreSSL doesn't have all of these. ['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 diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index c5a80b829e..08382e868b 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -397,6 +397,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 5a46cf0ee9..295b978525 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -885,6 +885,27 @@ check_expected_areq(AuthRequest areq, PGconn *conn) bool result = true; const char *reason = NULL; + 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) + { + appendPQExpBufferStr(&conn->errorMessage, + libpq_gettext("server did not request a certificate")); + return false; + } + else if (!conn->ssl_cert_sent) + { + appendPQExpBufferStr(&conn->errorMessage, + libpq_gettext("server accepted connection without a valid 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 completes diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 40f571fadb..eaca8cee1f 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)}, @@ -1514,6 +1520,55 @@ duplicate: 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; + appendPQExpBuffer(&conn->errorMessage, + libpq_gettext("invalid %s value: \"%s\"\n"), + "sslcertmode", + conn->sslcertmode); + return false; + } +#ifndef USE_SSL + if (strcmp(conn->sslcertmode, "require") == 0) + { + conn->status = CONNECTION_BAD; + appendPQExpBuffer(&conn->errorMessage, + libpq_gettext("sslcertmode value \"%s\" invalid when SSL support is not compiled in\n"), + 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 + * useless. + */ + if (strcmp(conn->sslcertmode, "require") == 0) + { + conn->status = CONNECTION_BAD; + appendPQExpBuffer(&conn->errorMessage, + libpq_gettext("sslcertmode value \"%s\" is not supported (check OpenSSL version)\n"), + conn->sslcertmode); + return false; + } +#endif + } + else + { + conn->sslcertmode = strdup(DefaultSSLCertMode); + if (!conn->sslcertmode) + goto oom_error; + } + /* * validate gssencmode option */ diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index b42a908733..04fa02af94 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -475,6 +475,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 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 @@ -970,6 +997,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); @@ -1133,7 +1165,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 e4d88e22a1..ad6f8b78c6 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 */ @@ -525,6 +526,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 efe5634fff..49003ba1b7 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 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/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm index c2acb58df0..ff6d6f9e35 100644 --- a/src/tools/msvc/Solution.pm +++ b/src/tools/msvc/Solution.pm @@ -328,6 +328,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, @@ -489,6 +490,13 @@ sub GenerateFiles my ($digit1, $digit2, $digit3) = $self->GetOpenSSLVersion(); + 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; + } + # More symbols are needed with OpenSSL 1.1.0 and above. if ( ($digit1 >= '3' && $digit2 >= '0' && $digit3 >= '0') || ($digit1 >= '1' && $digit2 >= '1' && $digit3 >= '0')) -- 2.25.1