Re: [PATCH] Reload SSL certificates on SIGHUP

On Mon, Nov 23, 2015 at 12:29 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> On 08/26/2015 07:46 AM, Michael Paquier wrote:
>>
>> On Wed, Aug 26, 2015 at 12:24 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>
>>> On Wed, Aug 26, 2015 at 10:57 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>>
>>>> [...]
>>>> So I think the way to move this forward is to investigate how to hold
>>>> the SSL config constant until SIGHUP in an EXEC_BACKEND build. If we
>>>> find out that that's unreasonably difficult, maybe we'll decide that
>>>> we can live without it; but I'd like to see the question investigated
>>>> rather than ignored.
>>>
>>>
>>> You have a point here.
>>>
>>> In EXEC_BACKEND, parameter updated via SIGHUP are only taken into
>>> account by newly-started backends, right?
>>
>>
>> Oops. I mistook with PGC_BACKEND here. Sorry for the noise.
>>
>>> Hence, a way to do what we
>>> want is to actually copy the data needed to initialize the SSL context
>>> into alternate file(s). When postmaster starts up, or when SIGHUP
>>> shows up those alternate files are upserted by the postmaster.
>>> be-secure-openssl.c needs also to be changed such as with EXEC_BACKEND
>>> the context needs to be loaded from those alternate files. At quick
>>> glance this seems doable.
>>
>>
>> Still, this idea would be to use a set of alternate files in global/
>> to set the context, basically something like
>> config_exec_ssl_cert_file, config_exec_ssl_key_file and
>> config_exec_ssl_ca_file. It does not seem to be necessary to
>> manipulate [read|write]_nondefault_variables() as the use of this
>> metadata should be made only when SSL context is initialized on
>> backend. Other thoughts welcome.
>
> I started implementing your suggested solution, but realized that I do not
> like copying of the private key file. The private key might have been put by
> the DBA on another file system for security reasons and having PostgreSQL
> copy potentially sensitive data to somewhere under pg_data seems like a
> surprising behavior. Especially since this only happens on some platforms.

You are referring for example to Fedora/Ubuntu that use a symlink to
point to those SSL files, right? Yes this approach may be a security
concern in those cases... One idea may be that we actually encrypt
this data

> I guess a possible solution would be to read the files into the postmaster
> (where we already have the private key today) and have OpenSSL read the keys
> from memory and re-implement something like
> SSL_CTX_use_certificate_chain_file() in our code, and similar things for the
> other functions which now take a path. This seems like a bit too much work
> to burden this patch with (and not obviously something we would want anyway)
> since the behavior is already different on Windows in the current code.
>
> Thoughts?

Reimplementing a twin of SSL_CTX_use_certificate_chain_file() would
have benefits in this case, but that's really something to avoid. I
may say something stupid here, but what if as you say we store the
information of the certificate into a dedicated shared memory block
when postmaster starts up, except that when we need to reload the keys
we dump them into a temporary file with tmpfile or similar and then
read it using SSL_CTX_use_certificate_chain_file(). For EXEC_BACKEND,
the shared memory block will be reattached at startup using
PGSharedMemoryReAttach() so it should have the data. For
non-EXEC_BACKEND, the child processes will just inherit the shmem
block with fork(). When SIGHUP is issued, all the processes
unconditionally dump the data into a per-process tmp file and then
reload it in the SSL context.
--
Michael