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.From 662bc1d19101865f91c6ed4a98c0f13f3757e1c7 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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.c
index 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
