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: Thu, 14 Jul 2022 22:12:33 +0200
Message-ID: <49b48719-d6cf-9e03-fab3-e4505cf9d585@enterprisedb.com> (raw)
In-Reply-To: <CAAWbhmiQVSAc_rOdW8q9R3GJehxkmaW5-CMbYLgrCjhotSs8Bg@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>
<2ef20161-e99d-a6ea-a36d-577843a2daae@enterprisedb.com>
<CAAWbhmiQVSAc_rOdW8q9R3GJehxkmaW5-CMbYLgrCjhotSs8Bg@mail.gmail.com>
On 13.07.22 01:06, Jacob Champion wrote:
>> 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?
>
> Yeah, I'm currently leaning heavily on the lack of any memory context
> switches here. And I end up leaking out a pointer to the stale stack
> of be_tls_open_server(), which is gross -- it works since there are no
> other clients, but that could probably come back to bite us.
>
> The ex_data API exposes optional callbacks for new/dup/free (I'm
> currently setting those to NULL), so we can run custom code whenever
> the SSL* is destroyed. If you'd rather the data have the same lifetime
> of the SSL* object, we can switch to malloc/strdup/free (or even
> OPENSSL_strdup() in later versions). But since we don't have any use
> for the ex_data outside of this function, maybe we should just clear
> it before we return, rather than carrying it around.
Concretely, I was thinking like the attached top-up patch.
The other way can surely be made to work somehow, but this seems much
simpler and with fewer questions about the details.
Attachments:v5-0002-squash-Log-details-for-client-certificate-failure.patch (text/plain, 3.5K) download
From 662bc1d19101865f91c6ed4a98c0f13f3757e1c7 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 14 Jul 2022 22:08:53 +0200
Subject: [PATCH v5 2/2] squash! Log details for client certificate failures
---
src/backend/libpq/be-secure-openssl.c | 29 +++++----------------------
1 file changed, 5 insertions(+), 24 deletions(-)
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.cindex 8d5eee0d16..2147524ffc 100644--- a/src/backend/libpq/be-secure-openssl.c+++ b/src/backend/libpq/be-secure-openssl.c@@ -81,13 +81,7 @@ 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;+static const char *cert_errdetail;
/* ------------------------------------------------------------ */
/* Public interface */
@@ -110,7 +104,6 @@ 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;
}
@@ -422,7 +415,6 @@ 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);
@@ -455,14 +447,6 @@ 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;
@@ -561,7 +545,7 @@ be_tls_open_server(Port *port)
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("could not accept SSL connection: %s",
SSLerrmessage(ecode)),
- exdata.errdetail ? errdetail_internal("%s", exdata.errdetail) : 0,+ cert_errdetail ? errdetail_internal("%s", cert_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 ?
@@ -570,6 +554,7 @@ be_tls_open_server(Port *port)
ssl_max_protocol_version ?
ssl_protocol_version_to_string(ssl_max_protocol_version) :
MAX_OPENSSL_TLS_VERSION) : 0));
+ cert_errdetail = NULL;
break;
case SSL_ERROR_ZERO_RETURN:
ereport(COMMERROR,
@@ -1145,12 +1130,10 @@ truncate_cert_name(char *name)
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)
@@ -1160,7 +1143,6 @@ verify_cb(int ok, X509_STORE_CTX *ctx)
}
/* 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);
@@ -1212,9 +1194,8 @@ verify_cb(int ok, X509_STORE_CTX *ctx)
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;+ /* Store our detail message to be logged later. */+ cert_errdetail = str.data;
return ok;
}
--
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: <49b48719-d6cf-9e03-fab3-e4505cf9d585@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