Re: Patch: Don't set LoadedSSL unless secure_initialize succeeds

On Mon, May 23, 2022 at 8:51 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
> >> On 22 May 2022, at 08:41, Gurjeet Singh <gurjeet(at)singh(dot)im> wrote:
> >> The initialization in PostmasterMain() blindly turns on LoadedSSL,
> >> irrespective of the outcome of secure_initialize().
>
> > This call is invoked with isServerStart set to true so any error in
> > secure_initialize should error out with ereport FATAL (in be_tls_init()). That
> > could be explained in a comment though, which is currently isn't.
>
> The comments for secure_initialize() and be_tls_init() both explain
> this already.

The comments above secure_initialize() do, but there are no comments
above be_tls_init(), and nothing in there attempts to explain the
FATAL vs. LOG difference.

I think it doesn't hurt, and may actively help, if we add a comment at
the call-site just alluding to the fact that the function call will
not return in case of an error, and that it's safe to assume certain
state has been initialized if the function returns.

The comments *inside* be_tls_init() attempt to explain allocation of a
new SSL_context, but end up making it sound like it's an optimization
to prevent memory leak. In the patch proposed a few minutes ago in
this thread, I have tried to explain above be_tls_init() the error
handling behavior, as well as the reason to retain the active
SSL_context, if any.

> It's not great that be_tls_init() implements two different error
> handling behaviors, perhaps. One could imagine separating those.
> But we've pretty much bought into such messes with the very fact
> that elog/ereport sometimes return and sometimes not.

I don't find the dual mode handling startling; I feel it's common in
Postgres code, but it's been a while since I've touched it.

What I would love to see improved around ereport() calls in SSL
functions would be to shrink the 'ereport(); goto error;' pattern into
one statement, so that we don't introduce an accidental "goto fail"
bug [1].

[1]: https://nakedsecurity.sophos.com/2014/02/24/anatomy-of-a-goto-fail-apples-ssl-bug-explained-plus-an-unofficial-patch/

Best regards,
Gurjeet
http://Gurje.et