On Sun, May 22, 2022 at 12:17 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> > On 22 May 2022, at 08:41, Gurjeet Singh <gurjeet@singh.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
