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

On Sun, May 22, 2022 at 12:17 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> > 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.

That makes sense. I have attached a patch that adds a couple of lines
of comments explaining this at call-site.

> Did you manage to get LoadedSSL set to true without SSL having been properly
> initialized?

Fortunately, no. I'm trying to add a new network protocol, and caught
this inconsistency while reading/adapting the code.

If a committer sees some value in it, in the attached patch I have
also attempted to improve the readability of the code a little bit in
startup-packet handling, and in SSL functions. These are purely
cosmetic changes, but I think they reduce repetition, and improve
readability, by quite a bit. For example, every ereport call evaluates
the same 'isServerStart ? FATAL : LOG', over and over again; replacing
this with variable 'logLevel' reduces cognitive load for the reader.
And I've replace one 'goto retry1' with a 'while' loop, like the
GASSAPI does it below that occurrence.

There's an symmetry, almost a diametric opposition, between how SSL
initialization error is treated when it occurs during server startup,
versus when the error occurs during a reload/SIGHUP. During startup an
error in SSL initialization leads to FATAL, whereas during a SIGHUP
it's merely a LOG message.

I found this difference in treatment of SSL initialization errors
quite bothersome, and there is no ready explanation for this. Either a
properly initialized SSL stack is important for server operation, or
it is not. What do we gain by letting the server operate normally
after a reload that failed to initialize SSL stack. Conversely, why do
we kill the server during startup on SSL initialization error, when
it's okay to operate normally after a reload that is unable to
initialize SSL.

I have added a comment to be_tls_init(), which I hope explains this
difference in treatment of errors. I have also added comments to
be_tls_init(), explaining why we don't destroy/free the global
SSL_context variable in case of an error in re-initialization of SSL;
it's not just an optimization, it's essential to normal server
operation.

Best regards,
Gurjeet
http://Gurje.et