Re: [PATCH] Reload SSL certificates on SIGHUP

On Thu, Nov 24, 2016 at 11:15 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> Never mind, I had not fetched the latest master. Once I had done so these
> tests were both broken in my rebased branch and in the master branch without
> any modifications. I guess I will have to bisect this once I get home.
>
> Inof for myself or anyone else who feels like bisecting this: the last known
> good commit is 67dc4ccbb2e1c27da823eced66d9217a5652cbb0.

Actually, it is a bit stupid of me to not move on with this patch as
this is backend-only, and the commit causing the regression failures
is 9a1d0af which is a frontend-only changes. So I have done a review
of this patch after reverting 9a1d0af, and things are working honestly
well.

Looking at the latest patch at code-level, there is some refactoring
to introduce initialize_context(). But it is actually not necessary
(perhaps this is the remnant of a past version?) as be_tls_init() is
its only caller. This patch makes hard to look at the diffs, and it
makes future back-patching more complicated, so I would suggest
simplifying the patch as much as possible. You could use for example a
goto block for the error code path to free the context with
SSL_CTX_free(), and set up ssl_loaded_verify_locations once the CA is
loaded. The same applies to initialize_ecdh().

+ if (secure_initialize() != 0)
+ ereport(FATAL,
+ (errmsg("could not load ssl context")));
+ LoadedSSL = true;
In case of a failure, a LOG message would have been already generated,
so this duplicates the information. Worse, if log_min_messages is set
to a level higher than LOG, users *lose* information on what has
happened. I think that secure_initialize() should have an argument to
define elevel and that this routine should be in charge of generating
an error message. Having it return a status code is necessary, but you
could cast secure_initialize() with (void) to show that we don't care
about the status code in this case. There is no need to care about
freeing the new context when the FATAL code path is used as process
would just shut down.

config.sgml needs to be updated to reflect that the SSL parameters are
updated at server reload (mentioned already upthread, just
re-mentioning it to group all the comments in one place).
--
Michael