From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
To: Jacob Champion <jchampion@timescale.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH] Log details for client certificate failures
Date: Mon, 11 Jul 2022 15:09:19 +0200
Message-ID: <2ef20161-e99d-a6ea-a36d-577843a2daae@enterprisedb.com> (raw)
In-Reply-To: <CAAWbhmgtVs0VciBwL4KbhQnLfTQKqwJjUUCxzA3SFECPn=ntTA@mail.gmail.com>
References: <d13c4a5787c2a3f83705124f0391e0738c796751.camel@vmware.com>
<cd03fa63-d4d9-8d3b-f9f5-13206a28d888@enterprisedb.com>
<a922e857c4e4a073f219bc5005b862eb22255c20.camel@vmware.com>
<290871e5-f1d9-54a8-5df4-dbd9e604c08d@enterprisedb.com>
<7c0468dcadbc0d0c987daf9ca08c8d111948eae6.camel@vmware.com>
<9b8338210b865d79239ec35ca523792ce331c0a3.camel@vmware.com>
<c60a8948-00a0-147d-75df-62699193371f@enterprisedb.com>
<CAAWbhmggsEacYW+cahN7USucsTgK9P4c5VMLaxWRdXvGQ=+BHA@mail.gmail.com>
<CAAWbhmiHyYajeN-SGRo_Nix+XbLtMyVkWinAoFVFy1kpbWT-zg@mail.gmail.com>
<61aa4b6c-9de2-d61b-821a-a34ab1e515e2@enterprisedb.com>
<CAAWbhmgtVs0VciBwL4KbhQnLfTQKqwJjUUCxzA3SFECPn=ntTA@mail.gmail.com>
On 08.07.22 20:39, Jacob Champion wrote:
> I also added an optional 0002 that bubbles the error info up to the
> final ereport(ERROR), using errdetail() and errhint(). I can squash it
> into 0001 if you like it, or drop it if you don't. (This approach
> could be adapted to the client, too.)
I squashed those two together. I also adjusted the error message a bit
more for project style. (We can put both lines into detail.)
I had to read up on this "ex_data" API. Interesting. But I'm wondering
a bit about how the life cycle of these objects is managed. What
happens if the allocated error string is deallocated before its
containing object? Or vice versa? How do we ensure we don't
accidentally reuse the error string when the code runs again? (I guess
currently it can't?) Maybe we should avoid this and just put the
errdetail itself into a static variable that we unset once the message
is printed?
Attachments:v5-0001-Log-details-for-client-certificate-failures.patch (text/plain, 16.6K) download
From e9bbf69618bfea433a6916af9cdcc59f04b96eda Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 11 Jul 2022 14:28:43 +0200
Subject: [PATCH v5] Log details for client certificate failures
Currently, debugging client cert verification failures is mostly
limited to looking at the TLS alert code on the client side. For
simple deployments, sometimes it's enough to see "sslv3 alert
certificate revoked" and know exactly what needs to be fixed, but if
you add any more complexity (multiple CA layers, misconfigured CA
certificates, etc.), trying to debug what happened based on the TLS
alert alone can be an exercise in frustration.
Luckily, the server has more information about exactly what failed in
the chain, and we already have the requisite callback implemented as a
stub. Add an ex_data pointer to the SSL handle, so that we can store
error information from the verification callback. This lets us add
our error details directly to the final "could not accept SSL
connection" log message, as opposed to issuing intermediate LOGs.
It ends up looking like
LOG: connection received: host=localhost port=43112
LOG: could not accept SSL connection: certificate verify failed
DETAIL: Client certificate verification failed at depth 1: unable to get local issuer certificate.
Failed certificate data (unverified): subject "/CN=Test CA for PostgreSQL SSL regression test client certs", serial number 2315134995201656577, issuer "/CN=Test root CA for PostgreSQL SSL regression test suite".
The length of the Subject and Issuer strings is limited to prevent
malicious client certs from spamming the logs. In case the truncation
makes things ambiguous, the certificate's serial number is also
logged.
Author: Jacob Champion <pchampion@vmware.com>
Discussion: https://www.postgresql.org/message-id/flat/d13c4a5787c2a3f83705124f0391e0738c796751.camel@vmware.com
---
src/backend/libpq/be-secure-openssl.c | 132 +++++++++++++++++++++++++-
src/test/ssl/conf/client-long.config | 14 +++
src/test/ssl/ssl/client-long.crt | 20 ++++
src/test/ssl/ssl/client-long.key | 27 ++++++
src/test/ssl/sslfiles.mk | 2 +-
src/test/ssl/t/001_ssltests.pl | 40 +++++++-
src/test/ssl/t/SSL/Backend/OpenSSL.pm | 2 +-
7 files changed, 230 insertions(+), 7 deletions(-)
create mode 100644 src/test/ssl/conf/client-long.config
create mode 100644 src/test/ssl/ssl/client-long.crt
create mode 100644 src/test/ssl/ssl/client-long.key
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.cindex 3d0168a369..8d5eee0d16 100644--- a/src/backend/libpq/be-secure-openssl.c+++ b/src/backend/libpq/be-secure-openssl.c@@ -81,6 +81,14 @@ static bool ssl_is_server_start;
static int ssl_protocol_version_to_openssl(int v);
static const char *ssl_protocol_version_to_string(int v);
+/* Helpers for storing application data in the SSL handle */+struct pg_ex_data+{+ /* to bubble errors out of callbacks */+ char *errdetail;+};+static int ssl_ex_index;+
/* ------------------------------------------------------------ */
/* Public interface */
/* ------------------------------------------------------------ */
@@ -102,6 +110,7 @@ be_tls_init(bool isServerStart)
SSL_library_init();
SSL_load_error_strings();
#endif
+ ssl_ex_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, NULL);
SSL_initialized = true;
}
@@ -413,6 +422,7 @@ be_tls_open_server(Port *port)
int err;
int waitfor;
unsigned long ecode;
+ struct pg_ex_data exdata = {0};
bool give_proto_hint;
Assert(!port->ssl);
@@ -445,6 +455,15 @@ be_tls_open_server(Port *port)
SSLerrmessage(ERR_get_error()))));
return -1;
}
+ if (!SSL_set_ex_data(port->ssl, ssl_ex_index, &exdata))+ {+ ereport(COMMERROR,+ (errcode(ERRCODE_PROTOCOL_VIOLATION),+ errmsg("could not attach application data to SSL handle: %s",+ SSLerrmessage(ERR_get_error()))));+ return -1;+ }+
port->ssl_in_use = true;
aloop:
@@ -537,10 +556,12 @@ be_tls_open_server(Port *port)
give_proto_hint = false;
break;
}
+
ereport(COMMERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("could not accept SSL connection: %s",
SSLerrmessage(ecode)),
+ exdata.errdetail ? errdetail_internal("%s", exdata.errdetail) : 0,
give_proto_hint ?
errhint("This may indicate that the client does not support any SSL protocol version between %s and %s.",
ssl_min_protocol_version ?
@@ -1076,12 +1097,46 @@ dummy_ssl_passwd_cb(char *buf, int size, int rwflag, void *userdata)
return 0;
}
+/*+ * Examines the provided certificate name, and if it's too long to log, modifies+ * and truncates it. The return value is NULL if no truncation was needed; it+ * otherwise points into the middle of the input string, and should not be+ * freed.+ */+static char *+truncate_cert_name(char *name)+{+ size_t namelen = strlen(name);+ char *truncated;++ /*+ * Common Names are 64 chars max, so for a common case where the CN is the+ * last field, we can still print the longest possible CN with a+ * 7-character prefix (".../CN=[64 chars]"), for a reasonable limit of 71+ * characters.+ */+#define MAXLEN 71++ if (namelen <= MAXLEN)+ return NULL;++ /*+ * Keep the end of the name, not the beginning, since the most specific+ * field is likely to give users the most information.+ */+ truncated = name + namelen - MAXLEN;+ truncated[0] = truncated[1] = truncated[2] = '.';++#undef MAXLEN++ return truncated;+}+
/*
* Certificate verification callback
*
- * This callback allows us to log intermediate problems during- * verification, but for now we'll see if the final error message- * contains enough information.+ * This callback allows us to examine intermediate problems during+ * verification, for later logging.
*
* This callback also allows us to override the default acceptance
* criteria (e.g., accepting self-signed or expired certs), but
@@ -1090,6 +1145,77 @@ dummy_ssl_passwd_cb(char *buf, int size, int rwflag, void *userdata)
static int
verify_cb(int ok, X509_STORE_CTX *ctx)
{
+ SSL *ssl;+ int depth;+ int errcode;+ const char *errstring;+ StringInfoData str;+ struct pg_ex_data *exdata;+ X509 *cert;++ if (ok)+ {+ /* Nothing to do for the successful case. */+ return ok;+ }++ /* Pull all the information we have on the verification failure. */+ ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx());+ depth = X509_STORE_CTX_get_error_depth(ctx);+ errcode = X509_STORE_CTX_get_error(ctx);+ errstring = X509_verify_cert_error_string(errcode);++ initStringInfo(&str);+ appendStringInfo(&str,+ _("Client certificate verification failed at depth %d: %s."),+ depth, errstring);++ cert = X509_STORE_CTX_get_current_cert(ctx);+ if (cert)+ {+ char *subject,+ *issuer;+ char *sub_truncated,+ *iss_truncated;+ char *serialno;+ ASN1_INTEGER *sn;+ BIGNUM *b;++ /*+ * Get the Subject and Issuer for logging, but don't let maliciously+ * huge certs flood the logs.+ */+ subject = X509_NAME_to_cstring(X509_get_subject_name(cert));+ sub_truncated = truncate_cert_name(subject);++ issuer = X509_NAME_to_cstring(X509_get_issuer_name(cert));+ iss_truncated = truncate_cert_name(issuer);++ /*+ * Pull the serial number, too, in case a Subject is still ambiguous.+ * This mirrors be_tls_get_peer_serial().+ */+ sn = X509_get_serialNumber(cert);+ b = ASN1_INTEGER_to_BN(sn, NULL);+ serialno = BN_bn2dec(b);++ appendStringInfoChar(&str, '\n');+ appendStringInfo(&str,+ _("Failed certificate data (unverified): subject \"%s\", serial number %s, issuer \"%s\"."),+ sub_truncated ? sub_truncated : subject,+ serialno ? serialno : _("unknown"),+ iss_truncated ? iss_truncated : issuer);++ BN_free(b);+ OPENSSL_free(serialno);+ pfree(issuer);+ pfree(subject);+ }++ /* Store our detail message in SSL application data, to be logged later. */+ exdata = SSL_get_ex_data(ssl, ssl_ex_index);+ exdata->errdetail = str.data;+
return ok;
}
diff --git a/src/test/ssl/conf/client-long.config b/src/test/ssl/conf/client-long.confignew file mode 100644index 0000000000..0e92a8fbfe--- /dev/null+++ b/src/test/ssl/conf/client-long.config@@ -0,0 +1,14 @@+# An OpenSSL format CSR config file for creating a client certificate with a+# long Subject.++[ req ]+distinguished_name = req_distinguished_name+prompt = no++[ req_distinguished_name ]+# Common Names are 64 characters max+CN = ssl-123456789012345678901234567890123456789012345678901234567890+OU = Some Organizational Unit+O = PostgreSQL Global Development Group++# no extensions in client certsdiff --git a/src/test/ssl/ssl/client-long.crt b/src/test/ssl/ssl/client-long.crtnew file mode 100644index 0000000000..a1db55b5c3--- /dev/null+++ b/src/test/ssl/ssl/client-long.crt@@ -0,0 +1,20 @@+-----BEGIN CERTIFICATE-----+MIIDWjCCAkICCCAiBRIUREYAMA0GCSqGSIb3DQEBCwUAMEIxQDA+BgNVBAMMN1Rl+c3QgQ0EgZm9yIFBvc3RncmVTUUwgU1NMIHJlZ3Jlc3Npb24gdGVzdCBjbGllbnQg+Y2VydHMwHhcNMjIwNTEyMjE0NDQ3WhcNNDkwOTI3MjE0NDQ3WjCBnDEsMCoGA1UE+CgwjUG9zdGdyZVNRTCBHbG9iYWwgRGV2ZWxvcG1lbnQgR3JvdXAxITAfBgNVBAsM+GFNvbWUgT3JnYW5pemF0aW9uYWwgVW5pdDFJMEcGA1UEAwxAc3NsLTEyMzQ1Njc4+OTAxMjM0NTY3ODkwMTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTIzNDU2+Nzg5MDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBANgxmeHiVRuBTwlG+Q1oM2M1ckQCI/o4hYcO9BYdxDYHiA7jy1WVenyj8BtUi5Aj9VDhpfiuewDarGQ5a+TggD1pMjkw0MorBKBr9+1u1xGH/8Q3lkgU+OQXrPglo4IrVcqaoZFQ0nuMaVbieX+0dDyTfsTaVQYYtqAtzhI/UGSIOhk2+lB9fP68jw9cLH0QYvR+qQ0IPG13I5zmSYP+Mj0VYwMn9TF9/2sBOSRVgTVAcrYgOQLk3s/fGe66tmVBIWYcq65ygqD1+weu+Pax+jPnwsefkdnf6JdYRG1F1Co7g52poPEYieAHfQOJ69sG0LYx0lBODC69qvSJ4WdCQ+0zKw288CAwEAATANBgkqhkiG9w0BAQsFAAOCAQEArr5r1UxgUzPykmu5ZdL6y8TA+ZbSQ1yBY0nhsRwRkDd66iPK9U6T6K2+pL8Vc6ioov9WOtHQ6ohP3gSavd40cHRmF+auwIsZ4Wk0mjftpOuPFp1hyo8d/QYrbEm3qNe5qln5S9h8ipoYvFtf5zlK2KHJFz+9ehZMZ1zGAERNCVM8UUQKyUuZB5GyrZlbslf6P/9Bsc54YUWxP2pr5r/RJ6DeXfI+zAFfXT8AFVlClARA949gpX0LVrXryDN60CUJ88QJmYCQ3AtIgzYYeqcdYHTd8eS2+9P5whDdU5NvROP+LjETeReJF4Bfyc2gM7zxZD2BDSf5exvnNqiy42/lR1b4szw==+-----END CERTIFICATE-----diff --git a/src/test/ssl/ssl/client-long.key b/src/test/ssl/ssl/client-long.keynew file mode 100644index 0000000000..5b455a021f--- /dev/null+++ b/src/test/ssl/ssl/client-long.key@@ -0,0 +1,27 @@+-----BEGIN RSA PRIVATE KEY-----+MIIEpAIBAAKCAQEA2DGZ4eJVG4FPCUZDWgzYzVyRAIj+jiFhw70Fh3ENgeIDuPLV+ZV6fKPwG1SLkCP1UOGl+K57ANqsZDlpOCAPWkyOTDQyisEoGv37W7XEYf/xDeWSB+T45Bes+CWjgitVypqhkVDSe4xpVuJ5fR0PJN+xNpVBhi2oC3OEj9QZIg6GTb6UH1+8/ryPD1wsfRBi9H6pDQg8bXcjnOZJg8yPRVjAyf1MX3/awE5JFWBNUBytiA5AuTe+z98Z7rq2ZUEhZhyrrnKCoPX7B6749rGM+fCx5+R2d/ol1hEbUXUKjuDnamg8RiJ4+Ad9A4nr2wbQtjHSUE4MLr2q9InhZ0JDTMrDbzwIDAQABAoIBADwJykpIqIny5xgU+QzAG0U52nm4fnVGrQ5MwMxDh/HZNZes+xLRaCqk/FEasYdd9Qp5H7Zn/hDGqYlLy+ESl4p2ZFQtkk4SlD5YvYladq+PrR+4sCtkZ5owWQCwsy+7CSAywRux7kIRRE+0pT+hxkXsUBAq8eG3i0AAeHHo01KX4kptlJ5d1pFKKAPThTUHCT4VPHg8r59IdsNy6wC++0E5ZRWsVUePy+ERuarX/um896hgbaiDJLFk02Orlc87+OBmRwO8J+KoUOEcAiTO+OZqGGaDEn5Y2mEdp2cCmq7+Izcklaha6CPsoV8+O2HK8PKvBIQmlgbDmal4/RNqr+JFqYz0ECgYEA+5z74Tmj+tzH57lcdMqVpndG39N8spBe8JbiFL16qOb6gRDytXjc+hY6IQo4JStpJulnPBZ5JQSbSBgCOzYWJJVBnnwMJKjNCd1th4znjxxMOe4LiDTtw+D3hQtzBU9FlI2sjWEUKf1xCyi9N41ApQC5eDWWd/0GN9+xAsxRjLL00CgYEA2/aH+4kNVsBHQ7vmv+sNsWeIgKg7PC7hRjcCYQG9ylBbBnFtv5XJYicXwqorqngzJPoGw+gB7iaSWL1UNAOSWRSFYe+woPpkY7n6Pbq211nzqV1avAdVrLylJwyE+EOQgTS30D+8BHv0I714PMd/QLK5NSUEr1IRtCfLeMpcSg6YYsCgYEAv3O86KxeTMTvyy9s3WVE+p4y8vhUDHi/iPbjhQBzJF3nhhJGrzE+xpGJG5jWDdpRQY15wuvqtDMkIKA8GmfWQ+3Hao0gKSV6z3VzCOdEKZQeILNAnsDVt7shm/eRRqoB7L48XLtQh37UJESUbY+qb6+L0fTZxTs2VjLBF1TY4mxGUUCgYEA1PLENKnJkA5/fowd8aA2CoKfbvgtPARyd9Bn+1aHPhEzPnabsGm7sBl2qFAEvCFoKjkgR7sd3nCHsUUetKmYTU7uEfLcN1YSS/oct+CLaMs92M53JCfZqsRrAvXc2VjX0i6Ocb49QJnph4tBHKC4MjmAuxWr8C9QPNxyfv+nAw9EOcCgYBYzejUzp6xiv5YzpwIncIF0A5E6VITcsW+LOR/FZOUPso0X2hQoIEs+wx8HjKCGfvX6628vnaWJC099hTmOzVwpEgik5zOmeAmZ//gt2I53Yg/loQUzH0CD+iXxrg/4Up7Yxx897w11ukOZv2xwmAFO1o52Q8k7d5FiMfEIzAkS3Pg==+-----END RSA PRIVATE KEY-----diff --git a/src/test/ssl/sslfiles.mk b/src/test/ssl/sslfiles.mkindex cc023667af..5f67aba795 100644--- a/src/test/ssl/sslfiles.mk+++ b/src/test/ssl/sslfiles.mk@@ -33,7 +33,7 @@ SERVERS := server-cn-and-alt-names \
server-multiple-alt-names \
server-no-names \
server-revoked
-CLIENTS := client client-dn client-revoked client_ext+CLIENTS := client client-dn client-revoked client_ext client-long
#
# To add a new non-standard key, add it to SPECIAL_KEYS and then add a recipe
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.plindex 707f4005af..90ba5a7515 100644--- a/src/test/ssl/t/001_ssltests.pl+++ b/src/test/ssl/t/001_ssltests.pl@@ -683,6 +683,10 @@ sub switch_server_cert
. sslkey('client-revoked.key'),
"certificate authorization fails with revoked client cert",
expected_stderr => qr/SSL error: sslv3 alert certificate revoked/,
+ log_like => [+ qr{Client certificate verification failed at depth 0: certificate revoked},+ qr{Failed certificate data \(unverified\): subject "/CN=ssltestuser", serial number 2315134995201656577, issuer "/CN=Test CA for PostgreSQL SSL regression test client certs"},+ ],
# revoked certificates should not authenticate the user
log_unlike => [qr/connection authenticated:/],);
@@ -730,7 +734,35 @@ sub switch_server_cert
$node->connect_fails(
$common_connstr . " " . "sslmode=require sslcert=ssl/client.crt",
"intermediate client certificate is missing",
- expected_stderr => qr/SSL error: tlsv1 alert unknown ca/);+ expected_stderr => qr/SSL error: tlsv1 alert unknown ca/,+ log_like => [+ qr{Client certificate verification failed at depth 0: unable to get local issuer certificate},+ qr{Failed certificate data \(unverified\): subject "/CN=ssltestuser", serial number 2315134995201656576, issuer "/CN=Test CA for PostgreSQL SSL regression test client certs"},+ ]);++$node->connect_fails(+ "$common_connstr sslmode=require sslcert=ssl/client-long.crt " . sslkey('client-long.key'),+ "logged client certificate Subjects are truncated if they're too long",+ expected_stderr => qr/SSL error: tlsv1 alert unknown ca/,+ log_like => [+ qr{Client certificate verification failed at depth 0: unable to get local issuer certificate},+ qr{Failed certificate data \(unverified\): subject "\.\.\./CN=ssl-123456789012345678901234567890123456789012345678901234567890", serial number 2315418733629425152, issuer "/CN=Test CA for PostgreSQL SSL regression test client certs"},+ ]);++# Use an invalid cafile here so that the next test won't be able to verify the+# client CA.+switch_server_cert($node, certfile => 'server-cn-only', cafile => 'server-cn-only');++# intermediate CA is provided but doesn't have a trusted root (checks error+# logging for cert chain depths > 0)+$node->connect_fails(+ "$common_connstr sslmode=require sslcert=ssl/client+client_ca.crt",+ "intermediate client certificate is untrusted",+ expected_stderr => qr/SSL error: tlsv1 alert unknown ca/,+ log_like => [+ qr{Client certificate verification failed at depth 1: unable to get local issuer certificate},+ qr{Failed certificate data \(unverified\): subject "/CN=Test CA for PostgreSQL SSL regression test client certs", serial number 2315134995201656577, issuer "/CN=Test root CA for PostgreSQL SSL regression test suite"},+ ]);
# test server-side CRL directory
switch_server_cert(
@@ -743,6 +775,10 @@ sub switch_server_cert
"$common_connstr user=ssltestuser sslcert=ssl/client-revoked.crt "
. sslkey('client-revoked.key'),
"certificate authorization fails with revoked client cert with server-side CRL directory",
- expected_stderr => qr/SSL error: sslv3 alert certificate revoked/);+ expected_stderr => qr/SSL error: sslv3 alert certificate revoked/,+ log_like => [+ qr{Client certificate verification failed at depth 0: certificate revoked},+ qr{Failed certificate data \(unverified\): subject "/CN=ssltestuser", serial number 2315134995201656577, issuer "/CN=Test CA for PostgreSQL SSL regression test client certs"},+ ]);
done_testing();
diff --git a/src/test/ssl/t/SSL/Backend/OpenSSL.pm b/src/test/ssl/t/SSL/Backend/OpenSSL.pmindex aed6005b43..a43e64c04f 100644--- a/src/test/ssl/t/SSL/Backend/OpenSSL.pm+++ b/src/test/ssl/t/SSL/Backend/OpenSSL.pm@@ -88,7 +88,7 @@ sub init
"client.key", "client-revoked.key",
"client-der.key", "client-encrypted-pem.key",
"client-encrypted-der.key", "client-dn.key",
- "client_ext.key");+ "client_ext.key", "client-long.key");
foreach my $keyfile (@keys)
{
copy("ssl/$keyfile", "$cert_tempdir/$keyfile")
--
2.37.0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: peter.eisentraut@enterprisedb.com, jchampion@timescale.com
Subject: Re: [PATCH] Log details for client certificate failures
In-Reply-To: <2ef20161-e99d-a6ea-a36d-577843a2daae@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox