Re: [PATCH] Log details for client certificate failures - Mailing list pgsql-hackers

From Jacob Champion
Subject Re: [PATCH] Log details for client certificate failures
Date
Msg-id CAAWbhmiQVSAc_rOdW8q9R3GJehxkmaW5-CMbYLgrCjhotSs8Bg@mail.gmail.com
Whole thread Raw
In response to Re: [PATCH] Log details for client certificate failures  (Peter Eisentraut <peter.eisentraut@enterprisedb.com>)
Responses Re: [PATCH] Log details for client certificate failures
List pgsql-hackers
On Mon, Jul 11, 2022 at 6:09 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> I squashed those two together.  I also adjusted the error message a bit
> more for project style.  (We can put both lines into detail.)

Oh, okay. Log parsers don't have any issues with that?

> 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.

> How do we ensure we don't
> accidentally reuse the error string when the code runs again?  (I guess
> currently it can't?)

The ex_data is associated with the SSL*, not the global SSL_CTX*, so
that shouldn't be an issue. A new SSL* gets created at the start of
be_tls_open_server().

>  Maybe we should avoid this and just put the
> errdetail itself into a static variable that we unset once the message
> is printed?

If you're worried about the lifetime of the palloc'd data being too
short, does switching to a static variable help in that case?

--Jacob



pgsql-hackers by date:

Previous
From: Jacob Champion
Date:
Subject: Re: [PATCH] Log details for client certificate failures
Next
From: Michael Paquier
Date:
Subject: Re: pg15b2: large objects lost on upgrade