Thread: re-reading SSL certificates during server reload

On Wed, Aug 27, 2014 at 11:56 AM, Alexey Klyukin <alexk@hintbits.com> wrote:
> Greetings,
>
> Is there a strong reason to disallow reloading server key and cert files
> during the PostgreSQL reload?

Key and cert files are loaded in the postmaster. We'd need to change
that. I'm not saying that's not a good idea, but it's not as easy as
just allowing it :)


> Basically, once you run multiple databases in a cluster and use different
> DNS names to connect to different databases (in order for those databases to
> be moved somewhere without changing the client code), and enable SSL
> certificate checking, the problem becomes evident: in order to add a new
> database to the existing cluster you have to add its name to the SSL
> certificate for the server, and in order for this changes to come into
> effect you have to restart the server.

That's certainly an issue. Potentially bigger ones are that you cannot
replace an expired certificate or CRL without a restart.


> In the documentation for server cert and key file there is a notice that
> this parameter can only be reloaded during the server start. It seems that
> the only place the backend certificates are loaded is inside the
> secure_initialize, which, in order, calls initialize_SSL().
>
> From my point of view, I see nothing preventing separation of the
> certificate reload code and SSL library initialization and calling the
> former during the server reload.  It might happen that with the new
> certificate file that some of the existing connections will be unable to
> reconnect, or, if the certificate is invalid, the server will be unable to
> restart, but this are the sort of problems that also happen with reload of
> pg_hba.conf as well, so these alone does not sound like a significant
> showstopper.

I agree that separating this out would probably be a useful idea. We
should probably treat a failed load of cerrtificates the same way we
do with pg_hba if we can - which is log an error and revert back to
the currently loaded one.

Some of this is going to have to be at least partially reworked anyway
in the work that Heikki has been diong to support non-openssl
libraries. Making a change like this at the same time is probably a
good idea.


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

* Magnus Hagander (magnus@hagander.net) wrote:
> That's certainly an issue. Potentially bigger ones are that you cannot
> replace an expired certificate or CRL without a restart.

+100.  I had forgotten about that issue- but it definitely sucks. :(

> Some of this is going to have to be at least partially reworked anyway
> in the work that Heikki has been diong to support non-openssl
> libraries. Making a change like this at the same time is probably a
> good idea.

Agreed.
Thanks,
    Stephen

On Wed, Aug 27, 2014 at 6:40 AM, Magnus Hagander <magnus@hagander.net> wrote:
> On Wed, Aug 27, 2014 at 11:56 AM, Alexey Klyukin <alexk@hintbits.com> wrote:
>> Greetings,
>>
>> Is there a strong reason to disallow reloading server key and cert files
>> during the PostgreSQL reload?
>
> Key and cert files are loaded in the postmaster. We'd need to change
> that.

Why?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Aug 28, 2014 at 3:20 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Aug 27, 2014 at 6:40 AM, Magnus Hagander <magnus@hagander.net> wrote:
>> On Wed, Aug 27, 2014 at 11:56 AM, Alexey Klyukin <alexk@hintbits.com> wrote:
>>> Greetings,
>>>
>>> Is there a strong reason to disallow reloading server key and cert files
>>> during the PostgreSQL reload?
>>
>> Key and cert files are loaded in the postmaster. We'd need to change
>> that.
>
> Why?

Hmm. That's actually a good point. Not sure I have an excuse. They
could certainly be made BACKEND without that, and there's no way to
change it within a running backend *anyway*, since we cannot turn
on/off SSL once a connection has been made. So yeah, it can actually
still be loaded in postmaster, and I withdraw that argument :)


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

Magnus Hagander <magnus@hagander.net> writes:
> On Thu, Aug 28, 2014 at 3:20 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Wed, Aug 27, 2014 at 6:40 AM, Magnus Hagander <magnus@hagander.net> wrote:
>>> Key and cert files are loaded in the postmaster. We'd need to change
>>> that.

>> Why?

> Hmm. That's actually a good point. Not sure I have an excuse. They
> could certainly be made BACKEND without that, and there's no way to
> change it within a running backend *anyway*, since we cannot turn
> on/off SSL once a connection has been made. So yeah, it can actually
> still be loaded in postmaster, and I withdraw that argument :)

Why would they need to be BACKEND, as opposed to just PGC_SIGHUP?
The only reason they're PGC_POSTMASTER is the lack of any code
for loading updated values, which I assume is something that's
possible with OpenSSL.

We could in fact wait to load them until after a backend has forked off
from the postmaster, but (1) that'd slow down session startup, and (2)
it would mean that you don't hear about broken settings at postmaster
startup.

(BTW, what happens on Windows?  I imagine we have to reload them anyway
after fork/exec on that platform ...)
        regards, tom lane

On Thu, Aug 28, 2014 at 4:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Thu, Aug 28, 2014 at 3:20 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Wed, Aug 27, 2014 at 6:40 AM, Magnus Hagander <magnus@hagander.net> wrote:
>>>> Key and cert files are loaded in the postmaster. We'd need to change
>>>> that.
>
>>> Why?
>
>> Hmm. That's actually a good point. Not sure I have an excuse. They
>> could certainly be made BACKEND without that, and there's no way to
>> change it within a running backend *anyway*, since we cannot turn
>> on/off SSL once a connection has been made. So yeah, it can actually
>> still be loaded in postmaster, and I withdraw that argument :)
>
> Why would they need to be BACKEND, as opposed to just PGC_SIGHUP?
> The only reason they're PGC_POSTMASTER is the lack of any code
> for loading updated values, which I assume is something that's
> possible with OpenSSL.

I just thought semantically - because they do not change in a running
backend. Any running backend will continue with encryption set up
based on the old certificate.


> We could in fact wait to load them until after a backend has forked off
> from the postmaster, but (1) that'd slow down session startup, and (2)
> it would mean that you don't hear about broken settings at postmaster
> startup.
>
> (BTW, what happens on Windows?  I imagine we have to reload them anyway
> after fork/exec on that platform ...)

Yes, we already do that - secure_initialize() is called in SubPostmasterMain().

But I think reloading them in the postmaster on Unix is the better choice, yes.

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

Magnus Hagander <magnus@hagander.net> writes:
> On Thu, Aug 28, 2014 at 4:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Why would they need to be BACKEND, as opposed to just PGC_SIGHUP?

> I just thought semantically - because they do not change in a running
> backend. Any running backend will continue with encryption set up
> based on the old certificate.

Hm.  Yeah, I guess there is some use in holding onto the values that were
actually used to initialize the current session, or at least there would
be if we exposed the cert contents in any fashion.
        regards, tom lane

On 2014-08-28 10:12:19 -0400, Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
> > On Thu, Aug 28, 2014 at 4:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Why would they need to be BACKEND, as opposed to just PGC_SIGHUP?
> 
> > I just thought semantically - because they do not change in a running
> > backend. Any running backend will continue with encryption set up
> > based on the old certificate.
> 
> Hm.  Yeah, I guess there is some use in holding onto the values that were
> actually used to initialize the current session, or at least there would
> be if we exposed the cert contents in any fashion.

Won't that allow the option to be specified at connection start by mere
mortal users? That sounds odd to me.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-08-28 10:12:19 -0400, Tom Lane wrote:
>> Hm.  Yeah, I guess there is some use in holding onto the values that were
>> actually used to initialize the current session, or at least there would
>> be if we exposed the cert contents in any fashion.

> Won't that allow the option to be specified at connection start by mere
> mortal users? That sounds odd to me.

Well, no, because SSL would be established (or not) before we ever process
the contents of the connection request packet.  You might be able to
change the value that SHOW reports, but not the value actually governing
your session.

Having said that, there's a nearby thread about inventing a "SUBACKEND"
GUC category, and that's likely what we'd really want to use here, just
on the grounds that superusers would know better.
        regards, tom lane

On Thu, Aug 28, 2014 at 4:14 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-08-28 10:12:19 -0400, Tom Lane wrote:
>> Magnus Hagander <magnus@hagander.net> writes:
>> > On Thu, Aug 28, 2014 at 4:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> >> Why would they need to be BACKEND, as opposed to just PGC_SIGHUP?
>>
>> > I just thought semantically - because they do not change in a running
>> > backend. Any running backend will continue with encryption set up
>> > based on the old certificate.
>>
>> Hm.  Yeah, I guess there is some use in holding onto the values that were
>> actually used to initialize the current session, or at least there would
>> be if we exposed the cert contents in any fashion.
>
> Won't that allow the option to be specified at connection start by mere
> mortal users? That sounds odd to me.

The cert is (and has to be) loaded before we even read the startup
packet, so there is no way for them to actually send the value over
early enough I believe.


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

On 2014-08-28 10:20:08 -0400, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2014-08-28 10:12:19 -0400, Tom Lane wrote:
> >> Hm.  Yeah, I guess there is some use in holding onto the values that were
> >> actually used to initialize the current session, or at least there would
> >> be if we exposed the cert contents in any fashion.
> 
> > Won't that allow the option to be specified at connection start by mere
> > mortal users? That sounds odd to me.
> 
> Well, no, because SSL would be established (or not) before we ever process
> the contents of the connection request packet.  You might be able to
> change the value that SHOW reports, but not the value actually governing
> your session.

Sure. There's probably nothing happening with ssl 'that late' right
now. But and it seems like a invitation for trouble later to me. Even if
it's just that other code copies the logic, thinking it'd also be safe.

> Having said that, there's a nearby thread about inventing a "SUBACKEND"
> GUC category, and that's likely what we'd really want to use here, just
> on the grounds that superusers would know better.

What we really want is PGC_SIGHUP | PGC_BACKEND, right? I wonder if it's
not better to allow for some cominations of GucContext's by bitmask,
instead of adding SUBACKEND and HUPBACKEND and whatever else might make
sense.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-08-28 10:20:08 -0400, Tom Lane wrote:
>> Having said that, there's a nearby thread about inventing a "SUBACKEND"
>> GUC category, and that's likely what we'd really want to use here, just
>> on the grounds that superusers would know better.

> What we really want is PGC_SIGHUP | PGC_BACKEND, right?

How you figure that?  You *don't* want it to change at SIGHUP, at least
not in existing sessions.  And the postmaster already thinks it should
reload PGC_BACKEND variables on SIGHUP.
        regards, tom lane

On 2014-08-28 10:30:30 -0400, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2014-08-28 10:20:08 -0400, Tom Lane wrote:
> >> Having said that, there's a nearby thread about inventing a "SUBACKEND"
> >> GUC category, and that's likely what we'd really want to use here, just
> >> on the grounds that superusers would know better.
> 
> > What we really want is PGC_SIGHUP | PGC_BACKEND, right?
> 
> How you figure that?  You *don't* want it to change at SIGHUP, at least
> not in existing sessions.  And the postmaster already thinks it should
> reload PGC_BACKEND variables on SIGHUP.

Well, it shouldn't be settable at connection start but changed for new
connections (expressed by _SIGHUP). But it shouldn't change for existing
connections (therefor PGC_BACKEND).

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services