Re: [PATCH] Reload SSL certificates on SIGHUP

On Tue, Jan 3, 2017 at 4:02 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> > Yeah, it seems that if you want to know whether you are using SSL, then
> > we already have that. I don't see the need for this new read-only
> setting.
>
> I concur --- there might be use for more reporting about SSL status, but
> that patch doesn't seem like quite the right thing.
>
> I've pushed the main patch with some small adjustments; one notable one
> that I made the EXEC_BACKEND reload path work like SIGHUP reload (press on
> without SSL if fail) not server start (abort if fail). As it stood, if
> you fat-fingered a change to your SSL files on Windows, the postmaster
> would keep running but all backends would die instantly, whether or not
> an SSL connection was being requested. That didn't seem helpful.
>
> I also went ahead and downgraded the "hostssl record with SSL turned off"
> case to a warning.
>
> Before we leave this area, though, there is a loose end that requires
> more thought. That is, what about passphrase-protected server keys?
> Our documentation suggests that if you have one, the server will demand
> the passphrase just once at server start and then all is good. I'm not
> sure if that's at all practical in modern usage, but in any case it's
> not going to be reasonable to put a passphrase in again at every SIGHUP.
> On Windows things are even worse; you'd have to give the passphrase again
> to every spawned backend. (But that was true already.)
>
> I can think of at least three things we might do about this:
>
> 1. Leave the code as it stands, and change the documentation to state
> that you cannot use a passphrase-protected server key, period.
>

2. Add a password callback function that would supply the passphrase
> when needed. The question is, where would it get that from? It'd
> be straightforward to supply it from a string GUC, but from a security
> POV it seems pretty silly to have the password in postgresql.conf.
>

Yeah, that seems like a really bad idea. If you want to do that, then you
might as well just remove the passphrase from the key.

> 3. Add a password callback function that just returns an empty string,
> thereby ensuring a clean failure if the server tries to load a
> passphrase-protected key. We'd still need to change the documentation
> but at least the behavior would be reasonably clean.
>
> I'm kinda leaning to the last choice; I don't want to leave things as they
> are, but actually making password-protected keys work in a useful way
> seems like a lot more effort than is justified.
>

The effort to deal with it may well be justified. IIRC, Greg Stark had some
ideas of what he wanted to do with encryption keys (or maybe that was
either somebody else or some other keys?), which also included getting the
private key out of the general postmaster address space to protect it. But
it *is* a bigger option, and in particular it is well out of scope of this
patch.

Of the three options I agree 3 is probably the best.

Another option would be to use a callback to get the key value the first
time, and then cache it so we can re-use it. That means we can make it work
if the new key has the same password, but it also means we need to take
care of protecting that passphrase. But I'm not sure that's any worse than
the fact that we keep the private key around unlocked today?

That said, they passphrase should only be required if the key changes, not
if the certificate changes, I believe. Do we take advantage of this today
(sorry, haven't looked at the code)? Because the most common operation is
probably the renewal of a certificate, which does not change the key, for
example...

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/