Re: [PATCH] Reload SSL certificates on SIGHUP

On Fri, Dec 2, 2016 at 1:02 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Dec 2, 2016 at 11:26 AM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
>> I have attached a new version. The commitfest should technically have been
>> closed by now, so do what you like with the status. I can always submit the
>> patch to the next commitfest.
>
> I have just moved it to the next CF. Will look at it in couple of
> days, that's more or less at the top of my TODO list.

Thanks for the new version, it is far easier to check that there is no
regression with the new code.

/* Public interface */
/* ------------------------------------------------------------ */

+
/*
Useless noise.

+void
+be_tls_destroy(void)
+{
+ SSL_CTX_free(SSL_context);
+ SSL_context = NULL;
}
Perhaps we had better leave immediately if SSL_context is already
NULL? I have tested the error code paths by enforcing manually an
error and I could not see any failures, still I am wondering if
calling SSL_CTX_free(NULL) would be safe, particularly if there is a
callback added in the future.

+ if (secure_initialize(false) != 0)
+ ereport(WARNING,
+ (errmsg("ssl context not reloaded")));
SSL should be upper-case here.

One last thing that I think is missing in this patch is for users the
possibility to check via SQL if the SSL context is actually loaded or
not. As the context is reloaded after all the new values are
available, with the current patch users may see that ssl is set to on
but no context is loaded. So why not adding for example a read-only
parameter that maps with SSLLoaded?

Once those issues are addressed, I think that this will be ready for committer.
--
Michael