Re: [PATCH] Reload SSL certificates on SIGHUP

On Mon, Nov 28, 2016 at 2:01 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Nov 24, 2016 at 11:15 PM, Andreas Karlsson <andreas@proxel.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).

As this patch could be really simplified this way, I am marking is as
returned with feedback.
-- 
Michael