Horton

Thread

Commits

  1. Re-allow SSL passphrase prompt at server start, but not thereafter.

  2. Disable prompting for passphrase while (re)loading SSL config files.

  3. Allow SSL configuration to be updated at SIGHUP.

  4. Add pg_sequences view

  1. [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2015-05-31T02:14:34Z

    Hi,

I have written a patch which makes it possible to change SSL 
certificates (and other SSL parameters, including the CRL) without 
restarting PostgreSQL. In fact this patch also makes it possible to turn 
on or off ssl entirely without restart. It does so by initializing a new 
SSL context when the postmaster receives a SIGHUP, and if the 
initialization succeeded the old context is replaced by the new.

There was some previous discussion[1] on the mailing list about what the 
proper context should be for the SSL parameters, but as far as I can 
tell the discussion never reached a conclusion. I have changed the SSL 
GUCs to PGC_SIGUP since I felt that was the closest to the truth, but it 
is not a perfect fit (the backends wont reload the SSL context). Should 
we add a new context for the SSL GUCs?

Notes

1. 
http://www.postgresql.org/message-id/flat/CAAS3tyLJcv-m0CqfMrrxUjwa9_FKscKuAKT9_L41wNuJZywM2Q@mail.gmail.com#CAAS3tyLJcv-m0CqfMrrxUjwa9_FKscKuAKT9_L41wNuJZywM2Q@mail.gmail.com

Andreas

  2. Re: [PATCH] Reload SSL certificates on SIGHUP

    Peter Eisentraut <peter_e@gmx.net> — 2015-07-02T16:13:54Z

    On 5/30/15 10:14 PM, Andreas Karlsson wrote:
> I have written a patch which makes it possible to change SSL
> certificates (and other SSL parameters, including the CRL) without
> restarting PostgreSQL. In fact this patch also makes it possible to turn
> on or off ssl entirely without restart. It does so by initializing a new
> SSL context when the postmaster receives a SIGHUP, and if the
> initialization succeeded the old context is replaced by the new.

I think this would be a useful feature, and the implementation looks
sound.  But I don't like how the reload is organized.  Reinitializing
the context in the sighup handler, aside from questions about how much
work one should do in a signal handler, would cause SSL reinitialization
for unrelated reloads.  We have the GUC assign hook mechanism for
handling this sort of thing.  The trick would be that when multiple
SSL-related settings change, you only want to do one reinitialization.
You could either have the different assign hooks communicate with each
other somehow, or have them set a "need SSL init" flag that is checked
somewhere else.

> There was some previous discussion[1] on the mailing list about what the
> proper context should be for the SSL parameters, but as far as I can
> tell the discussion never reached a conclusion. I have changed the SSL
> GUCs to PGC_SIGUP since I felt that was the closest to the truth, but it
> is not a perfect fit (the backends wont reload the SSL context). Should
> we add a new context for the SSL GUCs?

I think PGC_SIGHUP is fine for this.

  3. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2015-07-22T00:52:46Z

    On 07/02/2015 06:13 PM, Peter Eisentraut wrote:
> I think this would be a useful feature, and the implementation looks
> sound.  But I don't like how the reload is organized.  Reinitializing
> the context in the sighup handler, aside from questions about how much
> work one should do in a signal handler, would cause SSL reinitialization
> for unrelated reloads.  We have the GUC assign hook mechanism for
> handling this sort of thing.  The trick would be that when multiple
> SSL-related settings change, you only want to do one reinitialization.
> You could either have the different assign hooks communicate with each
> other somehow, or have them set a "need SSL init" flag that is checked
> somewhere else.

It is not enough to just add a hook to the GUCs since I would guess most 
users would expect the certificate to be reloaded if just the file has 
been replaced and no GUC was changed. To support this we would need to 
also check the mtimes of the SSL files, would that complexity really be 
worth it?

Andreas

  4. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Paquier <michael.paquier@gmail.com> — 2015-07-23T05:19:46Z

    On Wed, Jul 22, 2015 at 9:52 AM, Andreas Karlsson <andreas@proxel.se> wrote:
> On 07/02/2015 06:13 PM, Peter Eisentraut wrote:
>>
>> I think this would be a useful feature, and the implementation looks
>> sound.  But I don't like how the reload is organized.  Reinitializing
>> the context in the sighup handler, aside from questions about how much
>> work one should do in a signal handler, would cause SSL reinitialization
>> for unrelated reloads.  We have the GUC assign hook mechanism for
>> handling this sort of thing.  The trick would be that when multiple
>> SSL-related settings change, you only want to do one reinitialization.
>> You could either have the different assign hooks communicate with each
>> other somehow, or have them set a "need SSL init" flag that is checked
>> somewhere else.
>
>
> It is not enough to just add a hook to the GUCs since I would guess most
> users would expect the certificate to be reloaded if just the file has been
> replaced and no GUC was changed.

Why? It seems to me that the assign hook gets called once per process
at reload for a SIGHUP parameter even if its value is not changed, no?

> To support this we would need to also check
> the mtimes of the SSL files, would that complexity really be worth it?

Or we could reload the SSL context unconditionally once per reload
loop. I am wondering how costly that may prove to be though.
-- 
Michael

  5. Re: [PATCH] Reload SSL certificates on SIGHUP

    Peter Eisentraut <peter_e@gmx.net> — 2015-07-29T00:44:56Z

    On 7/21/15 8:52 PM, Andreas Karlsson wrote:
> It is not enough to just add a hook to the GUCs since I would guess most
> users would expect the certificate to be reloaded if just the file has
> been replaced and no GUC was changed. To support this we would need to
> also check the mtimes of the SSL files, would that complexity really be
> worth it?

Actually, I misread your patch.  I thought you only wanted to reload the
SSL files when the GUC settings change, but of course we also want to
reload them when the files are changed.

I don't have a problem with rebuilding the SSL context on every reload
cycle.  We already do a lot of extra reloading every time, so a bit more
shouldn't hurt.  But I'm not so sure whether we should do that in the
SIGHUP handler.  I don't know how we got into the situation of doing all
the file reloads directly in the handler, but at least we can control
that code.  Making a bunch of calls into an external library is a
different thing, though.  Can we find a way to do this differently?

  6. Re: [PATCH] Reload SSL certificates on SIGHUP

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-07-29T01:24:36Z

    Peter Eisentraut <peter_e@gmx.net> writes:
> I don't have a problem with rebuilding the SSL context on every reload
> cycle.  We already do a lot of extra reloading every time, so a bit more
> shouldn't hurt.  But I'm not so sure whether we should do that in the
> SIGHUP handler.  I don't know how we got into the situation of doing all
> the file reloads directly in the handler, but at least we can control
> that code.  Making a bunch of calls into an external library is a
> different thing, though.  Can we find a way to do this differently?

Do we have an idea how expensive it is to load that data?

A brute-force answer is to not have the postmaster load it at all,
but to have new backends do so (if needed) during their connection
acceptance/authentication phase.  I'm not sure how much that would
add to the SSL connection startup time though.  It would also mean
that problems with the SSL config files would only be reported during
subsequent connection starts, not at SIGHUP time, and indeed that
SIGHUP is more or less meaningless for SSL file changes: the instant
you change a file, it's live for later connections.  On the plus side,
it would make Windows and Unix behavior closer, since (I suppose)
we're reloading that stuff anyway in EXEC_BACKEND builds.

I'm not entirely sure your concern is valid, though.  We have always had
the principle that almost everything of interest in the postmaster happens
in signal handler functions.  We could possibly change things so that
reloading config files is done in the "main loop" of ServerLoop, but
if we did, it would have to execute with all signals blocked, which seems
like just about as much of a risk for third-party code as executing that
code in a signal handler is.

			regards, tom lane

  7. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2015-07-29T01:45:08Z

    
On 07/23/2015 07:19 AM, Michael Paquier wrote:
> On Wed, Jul 22, 2015 at 9:52 AM, Andreas Karlsson <andreas@proxel.se> wrote:
>> On 07/02/2015 06:13 PM, Peter Eisentraut wrote:
>>>
>>> I think this would be a useful feature, and the implementation looks
>>> sound.  But I don't like how the reload is organized.  Reinitializing
>>> the context in the sighup handler, aside from questions about how much
>>> work one should do in a signal handler, would cause SSL reinitialization
>>> for unrelated reloads.  We have the GUC assign hook mechanism for
>>> handling this sort of thing.  The trick would be that when multiple
>>> SSL-related settings change, you only want to do one reinitialization.
>>> You could either have the different assign hooks communicate with each
>>> other somehow, or have them set a "need SSL init" flag that is checked
>>> somewhere else.
>>
>>
>> It is not enough to just add a hook to the GUCs since I would guess most
>> users would expect the certificate to be reloaded if just the file has been
>> replaced and no GUC was changed.
>
> Why? It seems to me that the assign hook gets called once per process
> at reload for a SIGHUP parameter even if its value is not changed, no?

My bad, I tested it and you are correct. But I am not convinced moving 
the SSL initialization to a GUC assign hook would make anything clearer. 
It would not move any work out of the signal handler either since the 
assign hooks are ran inside it, and the hook will be ran in all backends 
which is not any interesting property for SSL initialization.

Andreas

  8. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Paquier <michael.paquier@gmail.com> — 2015-07-29T02:01:55Z

    On Wed, Jul 29, 2015 at 10:24 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Peter Eisentraut <peter_e@gmx.net> writes:
>> I don't have a problem with rebuilding the SSL context on every reload
>> cycle.  We already do a lot of extra reloading every time, so a bit more
>> shouldn't hurt.  But I'm not so sure whether we should do that in the
>> SIGHUP handler.  I don't know how we got into the situation of doing all
>> the file reloads directly in the handler, but at least we can control
>> that code.  Making a bunch of calls into an external library is a
>> different thing, though.  Can we find a way to do this differently?
>
> Do we have an idea how expensive it is to load that data?

There are no numbers on this thread. And honestly I would be curious
as well to see a run of pgbench with -C doing or similar to check how
long it takes to establish a connection. I would expect it to be
measurable though, but here I'm just hand-waving ;)

> A brute-force answer is to not have the postmaster load it at all,
> but to have new backends do so (if needed) during their connection
> acceptance/authentication phase.  I'm not sure how much that would
> add to the SSL connection startup time though.  It would also mean
> that problems with the SSL config files would only be reported during
> subsequent connection starts, not at SIGHUP time, and indeed that
> SIGHUP is more or less meaningless for SSL file changes: the instant
> you change a file, it's live for later connections.  On the plus side,
> it would make Windows and Unix behavior closer, since (I suppose)
> we're reloading that stuff anyway in EXEC_BACKEND builds.

Indeed.
-- 
Michael

  9. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2015-07-30T22:28:30Z

    On 07/29/2015 03:24 AM, Tom Lane wrote:
> Peter Eisentraut <peter_e@gmx.net> writes:
>> I don't have a problem with rebuilding the SSL context on every reload
>> cycle.  We already do a lot of extra reloading every time, so a bit more
>> shouldn't hurt.  But I'm not so sure whether we should do that in the
>> SIGHUP handler.  I don't know how we got into the situation of doing all
>> the file reloads directly in the handler, but at least we can control
>> that code.  Making a bunch of calls into an external library is a
>> different thing, though.  Can we find a way to do this differently?
>
> Do we have an idea how expensive it is to load that data?
>
> A brute-force answer is to not have the postmaster load it at all,
> but to have new backends do so (if needed) during their connection
> acceptance/authentication phase.  I'm not sure how much that would
> add to the SSL connection startup time though.  It would also mean
> that problems with the SSL config files would only be reported during
> subsequent connection starts, not at SIGHUP time, and indeed that
> SIGHUP is more or less meaningless for SSL file changes: the instant
> you change a file, it's live for later connections.  On the plus side,
> it would make Windows and Unix behavior closer, since (I suppose)
> we're reloading that stuff anyway in EXEC_BACKEND builds.

I measured it taking ~0.3ms to build the new SSL context in a simple 
benchmark (certificate + CA + small crl).

Personally I do not think moving this to connection start would be worth 
it since reporting errors that late is not nice for people who have 
misconfigured their database, and even if my benchmarks indicates it is 
relatively cheap to reload SSL adding more work to connection 
establishing is something I would want to avoid unless it gives us a 
clear benefit.

> I'm not entirely sure your concern is valid, though.  We have always had
> the principle that almost everything of interest in the postmaster happens
> in signal handler functions.  We could possibly change things so that
> reloading config files is done in the "main loop" of ServerLoop, but
> if we did, it would have to execute with all signals blocked, which seems
> like just about as much of a risk for third-party code as executing that
 > code in a signal handler is.

Agreed, I am not sure what moving it to the main loop would gain us.

-- 
Andreas

  10. Re: [PATCH] Reload SSL certificates on SIGHUP

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-08-26T01:57:51Z

    [ moving this discussion back to the patch thread ]

Andreas Karlsson <andreas@proxel.se> writes:
> On 08/25/2015 09:39 AM, Michael Paquier wrote:
>> -- Reload SSL certificates on SIGHUP: returned with feedback? I think
>> that this patch needs more work to be in a commitable state.

> Maybe I am being dense here, but I do not feel like I have gotten any 
> clear feedback which gives me a way forward with the patch. I do not 
> really see what more I can do here other than resubmit it to the next CF 
> which I feel would be poor etiquette by me.

I think we pretty much rejected Peter's concern about doing the work
in the SIGHUP handler.  There's been some other discussion about
refactoring the postmaster to not do all its work in signal handlers,
but that is material for a different patch.  Absent hard evidence that
reloading SSL config in the handler actually fails, I don't think we
should ask this patch to do a half-baked job of refactoring that.

However ... a concern I've got is that there's a difference between how
the Unix and Windows builds work, and this patch will move that from a
back-burner issue to a significant concern.  Namely, that on Unix we load
the SSL data once and that's what you use, while on Windows (or any
EXEC_BACKEND build) what you're going to get is whatever is in the files
right now when a connection starts, whether it's good or bad.  What this
patch does, unless I missed something, is to persuade the Unix ports to
implement "reload SSL data at SIGHUP", which is good; but the Windows
behavior stays where it is.

It didn't matter so much as long as changing the SSL config files wasn't
considered a supported operation; but if that is supported, then people
are going to complain.

Is it unreasonable of me to ask for the Windows behavior to be fixed at
the same time?  I dunno.  It's perhaps less broken than the Unix behavior,
but that doesn't make it desirable.  OTOH it might be a significantly
larger patch, and I confess I'm not even too sure what we'd have to do.

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.

			regards, tom lane

  11. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Paquier <michael.paquier@gmail.com> — 2015-08-26T03:24:27Z

    On Wed, Aug 26, 2015 at 10:57 AM, Tom Lane <tgl@sss.pgh.pa.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? 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.

For now I am moving the patch to the next CF, more investigation is
surely needed.
-- 
Michael

  12. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Paquier <michael.paquier@gmail.com> — 2015-08-26T05:46:54Z

    On Wed, Aug 26, 2015 at 12:24 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Aug 26, 2015 at 10:57 AM, Tom Lane <tgl@sss.pgh.pa.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.
-- 
Michael

  13. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2015-08-26T23:12:54Z

    On 08/26/2015 03:57 AM, Tom Lane wrote:
> Is it unreasonable of me to ask for the Windows behavior to be fixed at
> the same time?  I dunno.  It's perhaps less broken than the Unix behavior,
> but that doesn't make it desirable.  OTOH it might be a significantly
> larger patch, and I confess I'm not even too sure what we'd have to do.
>
> 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.

I think this is a real concern and one that I will look into, to see if 
it can be fixed with a reasonable amount of work.

Andreas

  14. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andres Freund <andres@anarazel.de> — 2015-10-03T15:26:20Z

    On 2015-08-27 01:12:54 +0200, Andreas Karlsson wrote:
> I think this is a real concern and one that I will look into, to see if it
> can be fixed with a reasonable amount of work.

This patch has been in waiting-for-author for a month. Marking it as
returned-with-feedback.

Greetings,

Andres Freund

  15. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2015-11-23T03:29:13Z

    On 08/26/2015 07:46 AM, Michael Paquier wrote:
> On Wed, Aug 26, 2015 at 12:24 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Wed, Aug 26, 2015 at 10:57 AM, Tom Lane <tgl@sss.pgh.pa.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.

Sorry for dropping this patch, but now I have started looking at it again.

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.

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?

I have attached a rebased version of the original patch which applies on 
current master.

Andreas

  16. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Paquier <michael.paquier@gmail.com> — 2015-11-26T06:25:06Z

    On Mon, Nov 23, 2015 at 12:29 PM, Andreas Karlsson <andreas@proxel.se> wrote:
> On 08/26/2015 07:46 AM, Michael Paquier wrote:
>>
>> On Wed, Aug 26, 2015 at 12:24 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>>
>>> On Wed, Aug 26, 2015 at 10:57 AM, Tom Lane <tgl@sss.pgh.pa.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

  17. Re: [PATCH] Reload SSL certificates on SIGHUP

    Peter Geoghegan <pg@heroku.com> — 2016-08-31T21:34:52Z

    On Sun, Nov 22, 2015 at 7:29 PM, Andreas Karlsson <andreas@proxel.se> wrote:
> Sorry for dropping this patch, but now I have started looking at it again.

Any chance of picking this up again soon, Andreas? I think it's an
important project. I would like to review it.


-- 
Peter Geoghegan

  18. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2016-09-04T14:39:10Z

    On 08/31/2016 11:34 PM, Peter Geoghegan wrote:
> On Sun, Nov 22, 2015 at 7:29 PM, Andreas Karlsson <andreas@proxel.se> wrote:
>> Sorry for dropping this patch, but now I have started looking at it again.
>
> Any chance of picking this up again soon, Andreas? I think it's an
> important project. I would like to review it.

I do not really have any good ideas for how to fix it for Windows, but 
if anyone would like to discuss solutions I am interested in working on 
this patch again.

The alternatives as I see them now:

1) Serialize the certificates, key, and CRL and write them to the 
backend_var temp file and then deserialize everything in the backends.

Sounds like you would need to write some code for every SSL library to 
support the serialization and deserialization, which I am not a fan of 
doing just for one platform since I worry about little used code paths. 
Additionally this would mean that we write a copy of the private key to 
potentially another file system than the one where the private key is 
stored, this sounds like a bad idea from a security point of view.

2) Copy all the SSL related files into the data directory at SIGHUP, 
before loading them. While this does not require any serialization of 
certificates it still has the problem of writing private keys to disk.

3) Leave my patch as it is now. This means the postmaster will reload 
certificates on SIGHUP while the backends will also load them when 
spawning. This means windows will continue to work the same as before my 
patch.

Is there any other way to pass the current set of loaded certificates 
and keys from the postmaster to the backends on Windows? I guess you 
could use a pipe, but if so we should probably send all data on this 
pipe, not just the SSL stuff.

I am leaning towards doing (3) but I know I am biased since it is less 
work and I do not care much for Windows.

Andreas

  19. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Paquier <michael.paquier@gmail.com> — 2016-09-07T08:09:17Z

    On Sun, Sep 4, 2016 at 11:39 PM, Andreas Karlsson <andreas@proxel.se> wrote:
> 1) Serialize the certificates, key, and CRL and write them to the
> backend_var temp file and then deserialize everything in the backends.
>
> Sounds like you would need to write some code for every SSL library to
> support the serialization and deserialization, which I am not a fan of doing
> just for one platform since I worry about little used code paths.
> Additionally this would mean that we write a copy of the private key to
> potentially another file system than the one where the private key is
> stored, this sounds like a bad idea from a security point of view.

Yeah... This would result in something that is heavily SSL-dependent,
which would be an additional maintenance pain when trying to support
future OpenSSL versions.

> 2) Copy all the SSL related files into the data directory at SIGHUP, before
> loading them. While this does not require any serialization of certificates
> it still has the problem of writing private keys to disk.

You expressed enough concern about that upthread, copying private keys
into PGDATA is a security concern.

> 3) Leave my patch as it is now. This means the postmaster will reload
> certificates on SIGHUP while the backends will also load them when spawning.
> This means windows will continue to work the same as before my patch.
>
> Is there any other way to pass the current set of loaded certificates and
> keys from the postmaster to the backends on Windows? I guess you could use a
> pipe, but if so we should probably send all data on this pipe, not just the
> SSL stuff.
>
> I am leaning towards doing (3) but I know I am biased since it is less work
> and I do not care much for Windows.

Seriously... The benefit of this feature is clear for a lot of people.
And the implementation dedicated only to Windows would just result in
a grotty thing anyway. So I'd say that at this point we could just
push for 3) and facilitate the life of most with SSL configuration.
The behavior across platforms needs to be properly documented for
sure.
-- 
Michael

  20. Re: [PATCH] Reload SSL certificates on SIGHUP

    Victor Wagner <vitus@wagner.pp.ru> — 2016-09-07T10:53:17Z

    On Wed, 7 Sep 2016 17:09:17 +0900
Michael Paquier <michael.paquier@gmail.com> wrote:

> On Sun, Sep 4, 2016 at 11:39 PM, Andreas Karlsson <andreas@proxel.se>
> wrote:
> > 1) Serialize the certificates, key, and CRL and write them to the
> > backend_var temp file and then deserialize everything in the
> > backends.
> >
> > Sounds like you would need to write some code for every SSL library
> > to support the serialization and deserialization, which I am not a
> > fan of doing just for one platform since I worry about little used
> > code paths. Additionally this would mean that we write a copy of
> > the private key to potentially another file system than the one
> > where the private key is stored, this sounds like a bad idea from a
> > security point of view.  
> 
> Yeah... This would result in something that is heavily SSL-dependent,
> which would be an additional maintenance pain when trying to support
> future OpenSSL versions.

OpenSSL has documented API for serializing/deserializing each and every
cryptographic format it supports. And this API quite unlikely to change
in future OpenSSL versions. Moreover, LibreSSL is compatible with this
API as far as I know.

Really, Apache does simular thing for ages - it starts as root, loads
certificates and keys, serializes them in memory, then forks and drops
privileges, and then uses these keys and certificates.

There are two problems with this approach

1. You have to carefully design data structures to store serialized
keys. Apache made a mistake there and didn't allow for future invention
of new public key algorithms.  So, in 2008 I had problems adding
russian GOST cryptography there.

2. You keys and certificates might not be stored in the filesystem at
all. They can live in some hardware cryptography module, which don't
let private keys out, just provide some handle.
(OpenSSL supports it via lodable engine modules, and Postgres allows to
use engine modules since 8.2, so people can use it with postgres).

Really OpenSSL/LibreSSL provide useful abstraction of memory BIO (Where
BIO stands for basic input/output) which can be used to store
serialized cryptographic data. And serializing/deserializing API is
designed to work with BIO anyway.

  21. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2016-10-31T09:40:18Z

    I have attached a version of the patch rebased on top of the OpenSSL 1.1 
changes.

Andreas

  22. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Banck <michael.banck@credativ.de> — 2016-11-03T14:24:05Z

    Hi,

this is a first review of this patch.

As a feature, I think this functionality is very welcome.  Having to
schedule a downtime in order to enable SSL or change the SSL certificate
is a nuisance and it might make admins think twice, reducing security.

The patch applies cleanly (modulo fuzz in the last hunk, but see below)
to master as of 00a86856, it compiles cleanly when using --with-openssl
and without that option and passes make check for both cases.

It does not update the documentation, I think at least section 18.9
"Secure TCP/IP Connections with SSL" needs updating as it says: "The
files server.key, server.crt, root.crt, and root.crl (or their
configured alternative names) are only examined during server start; so
you must restart the server for changes in them to take effect".

I (so far) lightly tested the functionality, and I could not find any
serious logic issues up till now. Changing or replacing the server
certificate with an expired one would make psql no longer connect with
sslmode=require. 

However see below for segfaults during testing.

Some code comments on the patch:

On Mon, Oct 31, 2016 at 10:40:18AM +0100, Andreas Karlsson wrote:
> diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
> index 668f217..a1b582f 100644
> --- a/src/backend/libpq/be-secure-openssl.c
> +++ b/src/backend/libpq/be-secure-openssl.c
> @@ -77,12 +77,14 @@ static DH  *generate_dh_parameters(int prime_len, int generator);
[...] 
> +	if ((context = initialize_context()) != NULL)
>  	{
[...]
> +		be_tls_destroy();

I am getting segfaults on reloading the configuration for this call.

|Program received signal SIGSEGV, Segmentation fault.
|X509_VERIFY_PARAM_free (param=0x21) at x509_vpm.c:105
|105	x509_vpm.c: Datei oder Verzeichnis nicht gefunden.
|(gdb) bt full
|#0  X509_VERIFY_PARAM_free (param=0x21) at x509_vpm.c:105
|No locals.
|#1  0x00007f3b59745a09 in SSL_CTX_free (a=0x21) at ssl_lib.c:1955
|        i = -1
|#2  0x00000000005ee314 in be_tls_destroy () at be-secure-openssl.c:197
|No locals.
|#3  be_tls_init () at be-secure-openssl.c:181
|No locals.
|#4  0x00000000005e3f55 in secure_initialize () at be-secure.c:74
|No locals.
|#5  0x000000000065f377 in SIGHUP_handler (postgres_signal_arg=<optimized out>) at postmaster.c:2524
|        save_errno = 4
|        __func__ = "SIGHUP_handler"
|#6  <signal handler called>
|No locals.
|#7  0x00007f3b58938873 in __select_nocancel () at ../sysdeps/unix/syscall-template.S:81
|No locals.
|#8  0x000000000046e027 in ServerLoop () at postmaster.c:1672
|        timeout = {tv_sec = 54, tv_usec = 568533}
|        rmask = {fds_bits = {56, 0 <repeats 15 times>}}
|        selres = <optimized out>
|        now = <optimized out>
|        readmask = {fds_bits = {56, 0 <repeats 15 times>}}
|        last_lockfile_recheck_time = 1478169442
|        last_touch_time = 1478169442
|        __func__ = "ServerLoop"
|#9  0x000000000066263e in PostmasterMain (argc=argc@entry=3, argv=argv@entry=0x2405df0)
|    at postmaster.c:1316
|        opt = <optimized out>
|        status = <optimized out>
|        userDoption = <optimized out>
|        listen_addr_saved = <optimized out>
|        i = <optimized out>
|        output_config_variable = <optimized out>
|        __func__ = "PostmasterMain"
|#10 0x000000000046f6b7 in main (argc=3, argv=0x2405df0) at main.c:228
|No locals.

What I'm doing is:

|postgres=# SHOW ssl;
| ssl 
|-----
| on
|(1 row)
|
|postgres=# ALTER SYSTEM SET ssl = off;
|ALTER SYSTEM
|postgres=# SELECT pg_reload_conf();
| pg_reload_conf 
|----------------
| t
|(1 row)
|
|postgres=# SHOW ssl;
| ssl 
|-----
| off
|(1 row)
|
|postgres=# ALTER SYSTEM SET ssl = on;
|ALTER SYSTEM
|postgres=# SELECT pg_reload_conf();
| pg_reload_conf 
|----------------
| t
|(1 row)
|
|postgres=# SHOW ssl;
|FATAL:  terminating connection due to unexpected postmaster exit
|server closed the connection unexpectedly
|	This probably means the server terminated abnormally
|	before or while processing the request.
|The connection to the server was lost. Attempting reset: Failed.
|!> 

The other one I got:

|Program received signal SIGSEGV, Segmentation fault.
|sk_pop_free (st=0x100007f00000002, func=0x7fdbac018ed0 <ASN1_OBJECT_free>) at stack.c:291
|291	stack.c: Datei oder Verzeichnis nicht gefunden.
|(gdb) bt full
|#0  sk_pop_free (st=0x100007f00000002, func=0x7fdbac018ed0 <ASN1_OBJECT_free>) at stack.c:291
|        i = <optimized out>
|#1  0x00007fdbac04324a in x509_verify_param_zero (param=0x10d4b40) at x509_vpm.c:85
|No locals.
|#2  X509_VERIFY_PARAM_free (param=0x10d4b40) at x509_vpm.c:105
|No locals.
|#3  0x00007fdbac33ca09 in SSL_CTX_free (a=0x100007f00000002) at ssl_lib.c:1955
|        i = -1
|#4  0x00000000005ee84c in be_tls_destroy () at be-secure-openssl.c:197
|No locals.
|#5  0x00000000005e3f65 in secure_destroy () at be-secure.c:87
|No locals.
|#6  0x000000000065f395 in SIGHUP_handler (postgres_signal_arg=<optimized out>) at postmaster.c:2532
|        save_errno = 4
|        __func__ = "SIGHUP_handler"
|#7  <signal handler called>
|No locals.
|#8  0x00007fdbab52f873 in __select_nocancel () at ../sysdeps/unix/syscall-template.S:81
|No locals.
|#9  0x000000000046e027 in ServerLoop () at postmaster.c:1672
|        timeout = {tv_sec = 54, tv_usec = 411921}
|        rmask = {fds_bits = {56, 0 <repeats 15 times>}}
|        selres = <optimized out>
|        now = <optimized out>
|        readmask = {fds_bits = {56, 0 <repeats 15 times>}}
|        last_lockfile_recheck_time = 1478182425
|        last_touch_time = 1478182425
|        __func__ = "ServerLoop"
|#10 0x000000000066263e in PostmasterMain (argc=argc@entry=4, argv=argv@entry=0x108de00)
|    at postmaster.c:1316
|        opt = <optimized out>
|        status = <optimized out>
|        userDoption = <optimized out>
|        listen_addr_saved = <optimized out>
|        i = <optimized out>
|        output_config_variable = <optimized out>
|        __func__ = "PostmasterMain"
|#11 0x000000000046f6b7 in main (argc=4, argv=0x108de00) at main.c:228
|No locals.

I ran pg_reload_conf() twice after setting ssl to off here:

|postgres=# SHOW ssl;
| ssl 
|-----
| on
|(1 row)
|
|postgres=# ALTER SYSTEM SET ssl = off;
|ALTER SYSTEM
|postgres=# SELECT pg_reload_conf();
| pg_reload_conf 
|----------------
| t
|(1 row)
|
|postgres=# SHOW ssl;
| ssl 
|-----
| off
|(1 row)
|
|postgres=# SELECT pg_reload_conf();
| pg_reload_conf 
|----------------
| t
|(1 row)
|
|postgres=# SHOW ssl;
|FATAL:  terminating connection due to unexpected postmaster exit
|server closed the connection unexpectedly
|	This probably means the server terminated abnormally
|	before or while processing the request.
|The connection to the server was lost. Attempting reset: Failed.

I set a breakpoint in be_tls_destroy() and dumped SSL_context in gdb for
both pg_reload_conf() calls and the differences are:

-$17 = {method = 0x7f4faf6bce80 <SSLv23_method_data.16018>, cipher_list = 0x298d3a0,
-  cipher_list_by_id = 0x298f140, cert_store = 0x298c4c0, sessions = 0x298c370, 
+$18 = {method = 0x7f4fae955b28 <main_arena+1288>, cipher_list = 0x7f4fae955b28 <main_arena+1288>, 
+  cipher_list_by_id = 0x298bf40, cert_store = 0x298bf40, sessions = 0x298c370, 
[...]
-    sess_timeout = 0, sess_cache_full = 0, sess_hit = 0, sess_cb_hit = 0}, references = 1,
+    sess_timeout = 0, sess_cache_full = 0, sess_hit = 0, sess_cb_hit = 0}, references = 0, 
[...]
-  comp_methods = 0x2969920, info_callback = 0x0, client_CA = 0x298cb80, options = 56098820, mode = 2,
+  comp_methods = 0x0, info_callback = 0x0, client_CA = 0x298cb80, options = 56098820, mode = 2, 

Not sure whether this might be an issue with my openssl library (this is
on Debian stable).

> +		SSL_context = context;
> +		/* Set flag to remember if CA store is successfully loaded */
> +		ssl_loaded_verify_locations = !!ssl_ca_file[0];

I am not sure this '!!' operator is according to project policy, it
seems to be not used elsewhere in the codebase except for imported or
auto-generated code. At least there should be a comment here, methinks.

[...]

> +
> +	/* set up the allowed cipher list */
> +	if (SSL_CTX_set_cipher_list(context, SSLCipherSuites) != 1)
> +		INIT_CONTEXT_ERROR((errmsg("could not set the cipher list (no valid ciphers available)")))

Missing semicolon at the end of the line.

[...]

> +static bool
> +initialize_ecdh(SSL_CTX *context)
>  {
>  #ifndef OPENSSL_NO_ECDH
>  	EC_KEY	   *ecdh;
>  	int			nid;
>  
>  	nid = OBJ_sn2nid(SSLECDHCurve);
> -	if (!nid)
> -		ereport(FATAL,
> +	if (!nid) {
> +		ereport(LOG,

Opening braces should be on the next line.

>  				(errmsg("ECDH: unrecognized curve name: %s", SSLECDHCurve)));
> +		return false;
> +	}
>  
>  	ecdh = EC_KEY_new_by_curve_name(nid);
> -	if (!ecdh)
> -		ereport(FATAL,
> +	if (!ecdh) {
> +		ereport(LOG,

Same.

> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
> index 24add74..c997581 100644
> --- a/src/backend/postmaster/postmaster.c
> +++ b/src/backend/postmaster/postmaster.c
> @@ -241,6 +241,8 @@ bool		enable_bonjour = false;
>  char	   *bonjour_name;
>  bool		restart_after_crash = true;
>  
> +static bool LoadedSSL = false;
> +

All the delarations above this one are global variables for GUCs (see
postmaster.h, lines 16-31).  So ISTM this static variable declaration
should be introduced by a comment in order to logically seperate it from
the previous ones, like the following static variables are:

>  /* PIDs of special child processes; 0 when not running */
>  static pid_t StartupPID = 0,
>  			BgWriterPID = 0,

[...]

> --- a/src/include/libpq/libpq.h
> +++ b/src/include/libpq/libpq.h
> @@ -85,6 +85,7 @@ extern int	(*pq_putmessage_hook) (char msgtype, const char *s, size_t len);
>  extern int	(*pq_flush_hook) (void);
>  
>  extern int	secure_initialize(void);
> +extern void secure_destroy(void);
>  extern bool secure_loaded_verify_locations(void);
>  extern void secure_destroy(void);
>  extern int	secure_open_server(Port *port);
> 

This hunk baffled me at first because two lines below your added
secure_destroy() declaration, the same line is already present.  I did
some digging and it turns out we had a secure_destroy() in the ancient
past, but its implementation got removed in 2008 in 4e8162865 as there
were no (more) users of it, however, the declaration was kept on until
now.

So this hunk should be removed I guess.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

  23. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2016-11-07T01:36:00Z

    Thanks for the review! I have fixed all your feedback in the attached patch.

On 11/03/2016 04:24 PM, Michael Banck wrote:
> It does not update the documentation, I think at least section 18.9
> "Secure TCP/IP Connections with SSL" needs updating as it says: "The
> files server.key, server.crt, root.crt, and root.crl (or their
> configured alternative names) are only examined during server start; so
> you must restart the server for changes in them to take effect".

Changed this.

> However see below for segfaults during testing.

Fixed, this was due to me not setting SSL_Context to NULL after freeing it.

>> [...]
>
> I am not sure this '!!' operator is according to project policy, it
> seems to be not used elsewhere in the codebase except for imported or
> auto-generated code. At least there should be a comment here, methinks.

I changed the code to compare with '\0' instead.

> [...]
> Missing semicolon at the end of the line.
> [...]
> Opening braces should be on the next line.
> [...]
> Same.

Fixed.

>> [...]
>
> All the delarations above this one are global variables for GUCs (see
> postmaster.h, lines 16-31).  So ISTM this static variable declaration
> should be introduced by a comment in order to logically seperate it from
> the previous ones, like the following static variables are:

Fixed.

>> [...]
>
> This hunk baffled me at first because two lines below your added
> secure_destroy() declaration, the same line is already present.  I did
> some digging and it turns out we had a secure_destroy() in the ancient
> past, but its implementation got removed in 2008 in 4e8162865 as there
> were no (more) users of it, however, the declaration was kept on until
> now.
>
> So this hunk should be removed I guess.

Removed.

Andreas

  24. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Banck <michael.banck@credativ.de> — 2016-11-08T12:22:41Z

    Hi,

On Mon, Nov 07, 2016 at 02:36:00AM +0100, Andreas Karlsson wrote:
> Thanks for the review! I have fixed all your feedback in the attached
> patch.

Thanks! I couldn't find furhter faults in my testing. I guess the
question what to do about this on Windows is possibly still open, but as
I am not familiar with the Windows port at all I've marked it Ready for
Committer for now.


Thanks,

Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

  25. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Paquier <michael.paquier@gmail.com> — 2016-11-08T13:15:29Z

    On Tue, Nov 8, 2016 at 9:22 PM, Michael Banck <michael.banck@credativ.de> wrote:
> Thanks! I couldn't find furhter faults in my testing. I guess the
> question what to do about this on Windows is possibly still open, but as
> I am not familiar with the Windows port at all I've marked it Ready for
> Committer for now.

The conclusion on Windows per upthread is "do nothing", because there
is no way to get a good balance between usability and security. To be
honest, I am fine with that.
-- 
Michael

  26. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2016-11-08T18:48:33Z

    On 11/08/2016 01:22 PM, Michael Banck wrote:
> Thanks! I couldn't find furhter faults in my testing. I guess the
> question what to do about this on Windows is possibly still open, but as
> I am not familiar with the Windows port at all I've marked it Ready for
> Committer for now.

Thanks again for the review!

Andreas

  27. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Paquier <michael.paquier@gmail.com> — 2016-11-09T05:54:45Z

    On Wed, Nov 9, 2016 at 3:48 AM, Andreas Karlsson <andreas@proxel.se> wrote:
> On 11/08/2016 01:22 PM, Michael Banck wrote:
>>
>> Thanks! I couldn't find furhter faults in my testing. I guess the
>> question what to do about this on Windows is possibly still open, but as
>> I am not familiar with the Windows port at all I've marked it Ready for
>> Committer for now.
>
> Thanks again for the review!

It seems to me that this patch is missing something... To begin with,
src/test/ssl/ServerSetup.pm should be patched so as the new SSL
configuration is reloaded after pg_ctl reload, and not after an
instance restart. That's straight-forward:
--- a/src/test/ssl/ServerSetup.pm
+++ b/src/test/ssl/ServerSetup.pm
@@ -96,7 +96,7 @@ sub configure_test_server_for_ssl
    close HBA;
 }

-# Change the configuration to use given server cert file, and restart
+# Change the configuration to use given server cert file, and reload
 # the server so that the configuration takes effect.
 sub switch_server_cert
 {
@@ -115,6 +115,6 @@ sub switch_server_cert
    print SSLCONF "ssl_crl_file='root+client.crl'\n";
    close SSLCONF;

-   # Stop and restart server to reload the new config.
-   $node->restart;
+   # Reload the new configuration set.
+   $node->reload;
 }

Once I did that, half of the tests are failing. And I would have
expected all of them to work properly.
-- 
Michael

  28. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2016-11-09T10:46:53Z

    On 11/09/2016 06:54 AM, Michael Paquier wrote:
> It seems to me that this patch is missing something... To begin with,
> src/test/ssl/ServerSetup.pm should be patched so as the new SSL
> configuration is reloaded after pg_ctl reload, and not after an
> instance restart. That's straight-forward:
> --- a/src/test/ssl/ServerSetup.pm
> +++ b/src/test/ssl/ServerSetup.pm
> @@ -96,7 +96,7 @@ sub configure_test_server_for_ssl
>     close HBA;
>  }
>
> -# Change the configuration to use given server cert file, and restart
> +# Change the configuration to use given server cert file, and reload
>  # the server so that the configuration takes effect.
>  sub switch_server_cert
>  {
> @@ -115,6 +115,6 @@ sub switch_server_cert
>     print SSLCONF "ssl_crl_file='root+client.crl'\n";
>     close SSLCONF;
>
> -   # Stop and restart server to reload the new config.
> -   $node->restart;
> +   # Reload the new configuration set.
> +   $node->reload;
>  }
>
> Once I did that, half of the tests are failing. And I would have
> expected all of them to work properly.

Those tests fail due to that listen_addresses cannot be changed on 
reload so none of the test cases can even connect to the database. When 
I hacked ServerSetup.pm to set the correct listen_address before 
starting all tests pass.

It is a bit annoying that if pg_hba.conf contains hostssl then postgres 
will refuse to start. Maybe this is something we should also fix in this 
patch since now when we can enable SSL after starting it becomes more 
useful to not bail on hostssl. What do you think?

I will look into writing a cleaner patch for ServerSetup.pm some time 
later this week.

Andreas

  29. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Paquier <michael.paquier@gmail.com> — 2016-11-10T06:16:53Z

    On Wed, Nov 9, 2016 at 7:46 PM, Andreas Karlsson <andreas@proxel.se> wrote:
> Those tests fail due to that listen_addresses cannot be changed on reload so
> none of the test cases can even connect to the database. When I hacked
> ServerSetup.pm to set the correct listen_address before starting all tests
> pass.

Hm... listen_addresses remain constant at 127.0.0.1 and setting up
listen_addresses = '*' does not work either.. Perhaps I am missing
something?

> It is a bit annoying that if pg_hba.conf contains hostssl then postgres will
> refuse to start. Maybe this is something we should also fix in this patch
> since now when we can enable SSL after starting it becomes more useful to
> not bail on hostssl. What do you think?

I forgot that... There is the same problem today when updating
postgresql.conf and restarting the server if there is an hostssl
entry. Do you have in mind to relax things? It seems to be that the
safest bet is to not reload parameters if ssl is switched from on to
off and if pg_hba.conf has a hostssl entry, right? That complicates
the code though.

> I will look into writing a cleaner patch for ServerSetup.pm some time later
> this week.

Thanks. Making the restart/reload OS-dependent will be necessary.
src/test/ssl can run on Windows.
-- 
Michael

  30. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2016-11-10T17:49:51Z

    On 11/10/2016 07:16 AM, Michael Paquier wrote:
> On Wed, Nov 9, 2016 at 7:46 PM, Andreas Karlsson <andreas@proxel.se> wrote:
>> Those tests fail due to that listen_addresses cannot be changed on reload so
>> none of the test cases can even connect to the database. When I hacked
>> ServerSetup.pm to set the correct listen_address before starting all tests
>> pass.
>
> Hm... listen_addresses remain constant at 127.0.0.1 and setting up
> listen_addresses = '*' does not work either.. Perhaps I am missing
> something?

When PostgreSQL is started in the tests it by default only listens to a 
unix socket (except on Windows). It is the call to the restart function 
in the SSL tests which allows PostgreSQL to receive TCP connections.

Fixing this in the SSL tests will require some refactoring.

>> It is a bit annoying that if pg_hba.conf contains hostssl then postgres will
>> refuse to start. Maybe this is something we should also fix in this patch
>> since now when we can enable SSL after starting it becomes more useful to
>> not bail on hostssl. What do you think?
>
> I forgot that... There is the same problem today when updating
> postgresql.conf and restarting the server if there is an hostssl
> entry. Do you have in mind to relax things? It seems to be that the
> safest bet is to not reload parameters if ssl is switched from on to
> off and if pg_hba.conf has a hostssl entry, right? That complicates
> the code though.

I personally think that it would be cleaner and easier to understand if 
we just do not fail on hostssl lines just because SSL is disabled. A 
warning should be enough. But I do not have any strong opinions here, 
and would be fine with leaving the code as-is.

>> I will look into writing a cleaner patch for ServerSetup.pm some time later
>> this week.
>
> Thanks. Making the restart/reload OS-dependent will be necessary.
> src/test/ssl can run on Windows.

I do not think that this will be an issue with the current design, but I 
do not have access to a Windows machine for testing.

Andreas

  31. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2016-11-11T18:40:02Z

    Hi,

Here is a new version of the patch with the only differences;

1) The SSL tests have been changed to use reload rather than restart

2) Rebased on master

Please take a look.

Andreas

  32. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2016-11-11T18:42:05Z

    On 11/11/2016 07:40 PM, Andreas Karlsson wrote:
> Hi,
>
> Here is a new version of the patch with the only differences;
>
> 1) The SSL tests have been changed to use reload rather than restart
>
> 2) Rebased on master

And here with a fix to a comment.

Andreas

  33. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Banck <michael.banck@credativ.de> — 2016-11-22T20:48:09Z

    Hi,

On Fri, Nov 11, 2016 at 07:42:05PM +0100, Andreas Karlsson wrote:
> On 11/11/2016 07:40 PM, Andreas Karlsson wrote:
> >Here is a new version of the patch with the only differences;
> >
> >1) The SSL tests have been changed to use reload rather than restart
> >
> >2) Rebased on master
> 
> And here with a fix to a comment.

Michael, as you brought up the issues with the SSL tests, do you plan to
review the latest version (and add yourself as reviewer)?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

  34. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Paquier <michael.paquier@gmail.com> — 2016-11-22T21:59:19Z

    On Wed, Nov 23, 2016 at 5:48 AM, Michael Banck
<michael.banck@credativ.de> wrote:
> On Fri, Nov 11, 2016 at 07:42:05PM +0100, Andreas Karlsson wrote:
>> On 11/11/2016 07:40 PM, Andreas Karlsson wrote:
>> >Here is a new version of the patch with the only differences;
>> >
>> >1) The SSL tests have been changed to use reload rather than restart
>> >
>> >2) Rebased on master
>>
>> And here with a fix to a comment.
>
> Michael, as you brought up the issues with the SSL tests, do you plan to
> review the latest version (and add yourself as reviewer)?

Yes. I need two days.
-- 
Michael

  35. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Paquier <michael.paquier@gmail.com> — 2016-11-24T07:46:19Z

    On Sat, Nov 12, 2016 at 3:42 AM, Andreas Karlsson <andreas@proxel.se> wrote:
> On 11/11/2016 07:40 PM, Andreas Karlsson wrote:
>> Here is a new version of the patch with the only differences;
>>
>> 1) The SSL tests have been changed to use reload rather than restart

Did you check if the tests pass? I am getting a couple of failures
like this one:
psql: server certificate for "common-name.pg-ssltest.test" does not
match host name "127.0.0.1"
not ok 11 - sslrootcert=ssl/root+server_ca.crt sslmode=verify-full
Attached are the logs of the run I did, and the same behavior shows
for macOS and Linux. The shape of the tests look correct to me after
review. Still, seeing failing tests with sslmode=verify-full is a
problem that needs to be addressed. This may be pointing to an
incorrect CA load handling, though I could not spot a problem when
going through the code.

>> 2) Rebased on master
>
> And here with a fix to a comment.

config.sgml needs an update as it still mentions that SSL parameter
require a restart when updated.

I have done a couple of tests on Linux, switching ssl mode between on
and off and testing connection attempts with sslmode. Things are
proving to work as I would expect them to be, so basically that's
nice:
- switching to off with sslmode=require triggers an error:
psql: server does not support SSL, but SSL was required
- switching to on with sslmode=require connects with SSL.
- switching to off with sslmode=prefer connects without SSL.
- switching to on with sslmode=prefer connects with SSL.

I have done as well a couple of tests with Windows, where switching
ssl between on and off is proving to work properly for each new
connection. There is no surprise here, and that's as documented in the
patch.
-- 
Michael

  36. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2016-11-24T13:49:34Z

    On 11/24/2016 08:46 AM, Michael Paquier wrote:
> On Sat, Nov 12, 2016 at 3:42 AM, Andreas Karlsson <andreas@proxel.se> wrote:
>> On 11/11/2016 07:40 PM, Andreas Karlsson wrote:
>>> Here is a new version of the patch with the only differences;
>>>
>>> 1) The SSL tests have been changed to use reload rather than restart
>
> Did you check if the tests pass? I am getting a couple of failures
> like this one:
> psql: server certificate for "common-name.pg-ssltest.test" does not
> match host name "127.0.0.1"
> not ok 11 - sslrootcert=ssl/root+server_ca.crt sslmode=verify-full
> Attached are the logs of the run I did, and the same behavior shows
> for macOS and Linux. The shape of the tests look correct to me after
> review. Still, seeing failing tests with sslmode=verify-full is a
> problem that needs to be addressed. This may be pointing to an
> incorrect CA load handling, though I could not spot a problem when
> going through the code.

Thanks for finding this. I will look at this more once I get home, but 
the tests do not fail on my computer. I wonder what I do differently.

What versions of Perl and OpenSSL do you run and how did you run the 
tests when the failed? I ran the tests by running "make check" inside 
"src/test/ssl".

Andreas

  37. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2016-11-24T14:15:34Z

    On 11/24/2016 02:49 PM, Andreas Karlsson wrote:
> Thanks for finding this. I will look at this more once I get home, but
> the tests do not fail on my computer. I wonder what I do differently.
>
> What versions of Perl and OpenSSL do you run and how did you run the
> tests when the failed? I ran the tests by running "make check" inside
> "src/test/ssl".

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.

Andreas

  38. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Paquier <michael.paquier@gmail.com> — 2016-11-28T05:01:11Z

    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).
-- 
Michael

  39. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Paquier <michael.paquier@gmail.com> — 2016-11-30T05:52:35Z

    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

  40. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2016-12-02T02:26:55Z

    On 11/30/2016 06:52 AM, Michael Paquier wrote:
> On Mon, Nov 28, 2016 at 2:01 PM, Michael Paquier
>> 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.

Thanks, this is a really good suggestion which made the diff much 
cleaner. I removed my new macro too now since I felt it mostly made the 
code more cryptic just to gain a few lines of code.

>> 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).

Thanks, fixed this.

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

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.

Andreas

  41. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Paquier <michael.paquier@gmail.com> — 2016-12-02T04:02:58Z

    On Fri, Dec 2, 2016 at 11:26 AM, Andreas Karlsson <andreas@proxel.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.
-- 
Michael

  42. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Paquier <michael.paquier@gmail.com> — 2016-12-04T13:12:24Z

    On Fri, Dec 2, 2016 at 1:02 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Dec 2, 2016 at 11:26 AM, Andreas Karlsson <andreas@proxel.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

  43. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2016-12-04T14:11:24Z

    On 12/04/2016 02:12 PM, Michael Paquier wrote:
> 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?

The other three issues are easy to fix, but this one is a bit trickier. 
Do you mean that we should add another GUC here which is read-only? Does 
this have a precedent in the code?

Andreas

  44. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Paquier <michael.paquier@gmail.com> — 2016-12-04T14:20:05Z

    On Sun, Dec 4, 2016 at 11:11 PM, Andreas Karlsson <andreas@proxel.se> wrote:
> On 12/04/2016 02:12 PM, Michael Paquier wrote:
>>
>> 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?
>
>
> The other three issues are easy to fix, but this one is a bit trickier. Do
> you mean that we should add another GUC here which is read-only?

Yes, that's what I meant. It is hard to track if the reloading has
been effective or not.

> Does this have a precedent in the code?

data_checksums in guc.c is an example, it is marked with
GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE and its value is updated with
SetConfigOption() when the control file is read. The idea would be to
do something like that with LoadedSSL.
-- 
Michael

  45. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2016-12-05T00:50:04Z

    On 12/04/2016 03:20 PM, Michael Paquier wrote:
> On Sun, Dec 4, 2016 at 11:11 PM, Andreas Karlsson <andreas@proxel.se> wrote:
>> On 12/04/2016 02:12 PM, Michael Paquier wrote:
>>>
>>> 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?
>>
>>
>> The other three issues are easy to fix, but this one is a bit trickier. Do
>> you mean that we should add another GUC here which is read-only?
>
> Yes, that's what I meant. It is hard to track if the reloading has
> been effective or not.
>
>> Does this have a precedent in the code?
>
> data_checksums in guc.c is an example, it is marked with
> GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE and its value is updated with
> SetConfigOption() when the control file is read. The idea would be to
> do something like that with LoadedSSL.

Thanks. I will be quite busy the upcoming couple of weeks so there will 
be a while until I can sit down with this. Feel free to contribute to 
the patch.

Andreas

  46. Re: [PATCH] Reload SSL certificates on SIGHUP

    Michael Paquier <michael.paquier@gmail.com> — 2016-12-05T05:17:48Z

    On Mon, Dec 5, 2016 at 9:50 AM, Andreas Karlsson <andreas@proxel.se> wrote:
> On 12/04/2016 03:20 PM, Michael Paquier wrote:
>> On Sun, Dec 4, 2016 at 11:11 PM, Andreas Karlsson <andreas@proxel.se>
>> wrote:
>>> On 12/04/2016 02:12 PM, Michael Paquier wrote:
>>> Does this have a precedent in the code?
>>
>>
>> data_checksums in guc.c is an example, it is marked with
>> GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE and its value is updated with
>> SetConfigOption() when the control file is read. The idea would be to
>> do something like that with LoadedSSL.

OK, here is attached what I had in mind as reload-ssl-v08-02.patch for
reference. This applies on top of the main patch
reload-ssl-v08-01.patch that is the same version as v7 with the issues
I reported previously as addressed. LoadedSSL is mapped with a
read-only GUC parameter that new sessions can query after connecting.
The only use case where that would be useful would be when using
sslmode=prefer to check whether the SSL context is loaded even if ssl
has been switched from off to on. But let's be honest, pg_stat_ssl
reports already this kind of information, making this patch at the end
useless because LoadedSSL does not change for an already-spawned
backend.

> Thanks. I will be quite busy the upcoming couple of weeks so there will be a
> while until I can sit down with this. Feel free to contribute to the patch.

I am switching the patch as ready for committer. I have no more
comments about this patch.

Note to committer-san: please look only at v08-01.
-- 
Michael

  47. Re: [PATCH] Reload SSL certificates on SIGHUP

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2016-12-14T14:59:24Z

    On 12/5/16 12:17 AM, Michael Paquier wrote:
> OK, here is attached what I had in mind as reload-ssl-v08-02.patch for
> reference. This applies on top of the main patch
> reload-ssl-v08-01.patch that is the same version as v7 with the issues
> I reported previously as addressed. LoadedSSL is mapped with a
> read-only GUC parameter that new sessions can query after connecting.
> The only use case where that would be useful would be when using
> sslmode=prefer to check whether the SSL context is loaded even if ssl
> has been switched from off to on. But let's be honest, pg_stat_ssl
> reports already this kind of information, making this patch at the end
> useless because LoadedSSL does not change for an already-spawned
> backend.

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.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  48. Re: [PATCH] Reload SSL certificates on SIGHUP

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-03T03:02:55Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.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.

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.

			regards, tom lane

  49. Re: [PATCH] Reload SSL certificates on SIGHUP

    Magnus Hagander <magnus@hagander.net> — 2017-01-03T07:46:16Z

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

> Peter Eisentraut <peter.eisentraut@2ndquadrant.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/

  50. Re: [PATCH] Reload SSL certificates on SIGHUP

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-01-03T14:34:38Z

    On 1/2/17 10:02 PM, Tom Lane wrote:
> Before we leave this area, though, there is a loose end that requires
> more thought.  That is, what about passphrase-protected server keys?

I don't have experience with this in practice, but my hunch would be
that you can continue to use passphrases as before, but the new reload
functionality is effectively not supported.  That is, if you use
passphrases and make a key change, you need to do a full restart.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  51. Re: [PATCH] Reload SSL certificates on SIGHUP

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-03T15:45:07Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 1/2/17 10:02 PM, Tom Lane wrote:
>> Before we leave this area, though, there is a loose end that requires
>> more thought.  That is, what about passphrase-protected server keys?

> I don't have experience with this in practice, but my hunch would be
> that you can continue to use passphrases as before, but the new reload
> functionality is effectively not supported.  That is, if you use
> passphrases and make a key change, you need to do a full restart.

Um, no, that's not the situation.  Since we reload the SSL_CTX each
time we get SIGHUP, and no information is carried forward from the
old SSL_CTX, a passphrase-protected server key causes the postmaster
to freeze at SIGHUP until somebody types a new passphrase on its
/dev/tty.  (I've just verified this: it stops accepting connections
or indeed doing anything at all.)  In the likely event that /dev/tty
no longer opens successfully, it would report failure of the SSL
reload and stop accepting new SSL connections.  Even if you thought
the previous behavior was useful, this completely isn't.

If there were a way to carry forward the decrypted key from the old
SSL_CTX to the new one, we could perhaps preserve the old behavior.
But I don't know of one, and even if there is one, it's likely very
OpenSSL-specific --- do we want to assume that every TLS implementation
will support that?

			regards, tom lane

  52. Re: [PATCH] Reload SSL certificates on SIGHUP

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-03T15:54:55Z

    Magnus Hagander <magnus@hagander.net> writes:
> On Tue, Jan 3, 2017 at 4:02 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Before we leave this area, though, there is a loose end that requires
>> more thought.  That is, what about passphrase-protected server keys?
>> ...
>> 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.

Agreed.  It's difficult to imagine a situation in which postgresql.conf
could be considered more secure than server.key, and usually it'd be less
so.

>> 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.

> 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?

Yeah, I'm not terribly fussed about having the passphrase sitting in
postmaster memory.  But the above is work I don't care to do ATM.

I think probably the right thing for now is to install a do-nothing
callback, so that at least we don't have the issue of the postmaster
freezing at SIGHUP.  If someone feels like trying to revive support
of passphrase-protected server keys, that would be a perfectly fine
base to build on; they'd need a callback there anyway.

> 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...

As I just explained to Peter, we don't have any way to exploit that given
the design of loading a whole new SSL_CTX.

			regards, tom lane

  53. Re: [PATCH] Reload SSL certificates on SIGHUP

    Magnus Hagander <magnus@hagander.net> — 2017-01-04T13:46:11Z

    On Tue, Jan 3, 2017 at 4:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Magnus Hagander <magnus@hagander.net> writes:
> > On Tue, Jan 3, 2017 at 4:02 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Before we leave this area, though, there is a loose end that requires
> >> more thought.  That is, what about passphrase-protected server keys?
> >> ...
> >> 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.
>
> Agreed.  It's difficult to imagine a situation in which postgresql.conf
> could be considered more secure than server.key, and usually it'd be less
> so.
>
> >> 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.
>
> > 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?
>
> Yeah, I'm not terribly fussed about having the passphrase sitting in
> postmaster memory.  But the above is work I don't care to do ATM.
>
> I think probably the right thing for now is to install a do-nothing
> callback, so that at least we don't have the issue of the postmaster
> freezing at SIGHUP.  If someone feels like trying to revive support
> of passphrase-protected server keys, that would be a perfectly fine
> base to build on; they'd need a callback there anyway.
>

Does it still support passphrase protected ones on startup, or did that get
thrown out with the bathwater? I think that's definitely a separate thing
and there are a nontrivial number of people who would be interested in a
setup where they can use a passphrase to protect it initially, just not
reload it.



>
> > 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...
>
> As I just explained to Peter, we don't have any way to exploit that given
> the design of loading a whole new SSL_CTX.
>

Bummer.


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

  54. Re: [PATCH] Reload SSL certificates on SIGHUP

    Stephen Frost <sfrost@snowman.net> — 2017-01-04T13:48:36Z

    * Magnus Hagander (magnus@hagander.net) wrote:
> On Tue, Jan 3, 2017 at 4:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> > Magnus Hagander <magnus@hagander.net> writes:
> > > On Tue, Jan 3, 2017 at 4:02 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >> Before we leave this area, though, there is a loose end that requires
> > >> more thought.  That is, what about passphrase-protected server keys?
> > >> ...
> > >> 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.
> >
> > Agreed.  It's difficult to imagine a situation in which postgresql.conf
> > could be considered more secure than server.key, and usually it'd be less
> > so.
> >
> > >> 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.
> >
> > > 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?
> >
> > Yeah, I'm not terribly fussed about having the passphrase sitting in
> > postmaster memory.  But the above is work I don't care to do ATM.
> >
> > I think probably the right thing for now is to install a do-nothing
> > callback, so that at least we don't have the issue of the postmaster
> > freezing at SIGHUP.  If someone feels like trying to revive support
> > of passphrase-protected server keys, that would be a perfectly fine
> > base to build on; they'd need a callback there anyway.
> 
> Does it still support passphrase protected ones on startup, or did that get
> thrown out with the bathwater? I think that's definitely a separate thing
> and there are a nontrivial number of people who would be interested in a
> setup where they can use a passphrase to protect it initially, just not
> reload it.

+1

Thanks!

Stephen

  55. Re: [PATCH] Reload SSL certificates on SIGHUP

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-04T14:47:10Z

    Magnus Hagander <magnus@hagander.net> writes:
> On Tue, Jan 3, 2017 at 4:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think probably the right thing for now is to install a do-nothing
>> callback, so that at least we don't have the issue of the postmaster
>> freezing at SIGHUP.  If someone feels like trying to revive support
>> of passphrase-protected server keys, that would be a perfectly fine
>> base to build on; they'd need a callback there anyway.

> Does it still support passphrase protected ones on startup, or did that get
> thrown out with the bathwater?

It does not; what would be the point, if the key would be lost at SIGHUP?

> I think that's definitely a separate thing
> and there are a nontrivial number of people who would be interested in a
> setup where they can use a passphrase to protect it initially, just not
> reload it.

If any of those number of people want to step up and design/implement
a non-broken solution for passphrases, that'd be fine with me.  But
I would want to see something that's actually a credible solution,
allowing the postmaster to be started as a normal daemon.  And working
on Windows.

			regards, tom lane

  56. Re: [PATCH] Reload SSL certificates on SIGHUP

    Magnus Hagander <magnus@hagander.net> — 2017-01-04T14:48:55Z

    On Wed, Jan 4, 2017 at 3:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Magnus Hagander <magnus@hagander.net> writes:
> > On Tue, Jan 3, 2017 at 4:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I think probably the right thing for now is to install a do-nothing
> >> callback, so that at least we don't have the issue of the postmaster
> >> freezing at SIGHUP.  If someone feels like trying to revive support
> >> of passphrase-protected server keys, that would be a perfectly fine
> >> base to build on; they'd need a callback there anyway.
>
> > Does it still support passphrase protected ones on startup, or did that
> get
> > thrown out with the bathwater?
>
> It does not; what would be the point, if the key would be lost at SIGHUP?
>

If we lost it, yes. But we could keep the old key around if it hasn't
changed, thus behave just like we did in <= 9.6.



> > I think that's definitely a separate thing
> > and there are a nontrivial number of people who would be interested in a
> > setup where they can use a passphrase to protect it initially, just not
> > reload it.
>
> If any of those number of people want to step up and design/implement
> a non-broken solution for passphrases, that'd be fine with me.  But
> I would want to see something that's actually a credible solution,
> allowing the postmaster to be started as a normal daemon.  And working
> on Windows.
>

Well, for all those people 9.6 worked significantly better... Because they
could reload *other* config parameters without failure.

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

  57. Re: [PATCH] Reload SSL certificates on SIGHUP

    Stephen Frost <sfrost@snowman.net> — 2017-01-04T14:59:25Z

    * Magnus Hagander (magnus@hagander.net) wrote:
> On Wed, Jan 4, 2017 at 3:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Magnus Hagander <magnus@hagander.net> writes:
> > > On Tue, Jan 3, 2017 at 4:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >> I think probably the right thing for now is to install a do-nothing
> > >> callback, so that at least we don't have the issue of the postmaster
> > >> freezing at SIGHUP.  If someone feels like trying to revive support
> > >> of passphrase-protected server keys, that would be a perfectly fine
> > >> base to build on; they'd need a callback there anyway.
> >
> > > Does it still support passphrase protected ones on startup, or did that
> > get
> > > thrown out with the bathwater?
> >
> > It does not; what would be the point, if the key would be lost at SIGHUP?
> 
> If we lost it, yes. But we could keep the old key around if it hasn't
> changed, thus behave just like we did in <= 9.6.

Either that, or throw the same warning that we can't reload the SSL
bits if we had to ask for a passphrase at startup, again like how it
worked in 9.6.

> > > I think that's definitely a separate thing
> > > and there are a nontrivial number of people who would be interested in a
> > > setup where they can use a passphrase to protect it initially, just not
> > > reload it.
> >
> > If any of those number of people want to step up and design/implement
> > a non-broken solution for passphrases, that'd be fine with me.  But
> > I would want to see something that's actually a credible solution,
> > allowing the postmaster to be started as a normal daemon.  And working
> > on Windows.
> 
> Well, for all those people 9.6 worked significantly better... Because they
> could reload *other* config parameters without failure.

Indeed, this is important functionality that people are using.  I don't
agree with simply removing it because we want to make these options able
to be changed on a reload, that's not a good trade-off.

Thanks!

Stephen

  58. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2017-01-04T15:01:22Z

    On 01/04/2017 03:48 PM, Magnus Hagander wrote:
> On Wed, Jan 4, 2017 at 3:47 PM, Tom Lane <tgl@sss.pgh.pa.us
>     It does not; what would be the point, if the key would be lost at
>     SIGHUP?
>
> If we lost it, yes. But we could keep the old key around if it hasn't
> changed, thus behave just like we did in <= 9.6.

That means storing the pass phrase in the memory of the postmaster, 
which does not sound like a terribly good idea to me, but I have never 
used keys with pass phrases for daemons so it might be a common solution 
which is acceptable by many.

>     If any of those number of people want to step up and design/implement
>     a non-broken solution for passphrases, that'd be fine with me.  But
>     I would want to see something that's actually a credible solution,
>     allowing the postmaster to be started as a normal daemon.  And working
>     on Windows.
>
> Well, for all those people 9.6 worked significantly better... Because
> they could reload *other* config parameters without failure.

A possible solution might be to only add the error throwing hook when 
loading certificates during SIGHUP (and at Windows) and to work as 
before on startup. Would that be an acceptable solution? I could write a 
patch for this if people are interested.

Andreas

  59. Re: [PATCH] Reload SSL certificates on SIGHUP

    Stephen Frost <sfrost@snowman.net> — 2017-01-04T15:14:29Z

    * Andreas Karlsson (andreas@proxel.se) wrote:
> On 01/04/2017 03:48 PM, Magnus Hagander wrote:
> >On Wed, Jan 4, 2017 at 3:47 PM, Tom Lane <tgl@sss.pgh.pa.us
> >    It does not; what would be the point, if the key would be lost at
> >    SIGHUP?
> >
> >If we lost it, yes. But we could keep the old key around if it hasn't
> >changed, thus behave just like we did in <= 9.6.
> 
> That means storing the pass phrase in the memory of the postmaster,
> which does not sound like a terribly good idea to me, but I have
> never used keys with pass phrases for daemons so it might be a
> common solution which is acceptable by many.

I'm not sure I see that as a big deal- if you can access the
postmaster's memory then you can just pull out the actual private key
itself, no?  There's a bigger issue here though, which has to do with
what happens if the user actually does change the passphrase on the key
and we then reload, what do we do then?

> >    If any of those number of people want to step up and design/implement
> >    a non-broken solution for passphrases, that'd be fine with me.  But
> >    I would want to see something that's actually a credible solution,
> >    allowing the postmaster to be started as a normal daemon.  And working
> >    on Windows.
> >
> >Well, for all those people 9.6 worked significantly better... Because
> >they could reload *other* config parameters without failure.
> 
> A possible solution might be to only add the error throwing hook
> when loading certificates during SIGHUP (and at Windows) and to work
> as before on startup. Would that be an acceptable solution? I could
> write a patch for this if people are interested.

I'm not sure I see how that's a solution..?  Wouldn't that mean that a
SIGHUP with an encrypted key would result in a failure?

The solution, at least in my view, seems to be to say "sorry, we can't
reload the SSL stuff if you used a passphrase to unlock the key on
startup, you will have to perform a restart if you want the SSL bits to
be changed."

No, I've not looked at what that would require in terms of code.

Thanks!

Stephen

  60. Re: [PATCH] Reload SSL certificates on SIGHUP

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-04T15:17:23Z

    Stephen Frost <sfrost@snowman.net> writes:
> Indeed, this is important functionality that people are using.

Who exactly are these people, and why haven't they complained about how
crappy the support is now?  I'm *completely* unconvinced by the argument
that the way it has worked in the past is an important feature that we
have to preserve.  It's an accident that it worked at all, and as far as
I can tell it didn't work very well.  Have you tried it?  On my machine,
it could not accept a passphrase at all unless I didn't detach the
postmaster from the terminal, which is entirely silly as a production
solution.

In short, I reject the above argument 100%.  I'm all for inventing
a solution in which passphrases work usefully, but don't tell me
that what we had was such a solution.

			regards, tom lane

  61. Re: [PATCH] Reload SSL certificates on SIGHUP

    Andreas Karlsson <andreas@proxel.se> — 2017-01-04T15:17:49Z

    On 01/04/2017 04:14 PM, Stephen Frost wrote:
> * Andreas Karlsson (andreas@proxel.se) wrote:
>> A possible solution might be to only add the error throwing hook
>> when loading certificates during SIGHUP (and at Windows) and to work
>> as before on startup. Would that be an acceptable solution? I could
>> write a patch for this if people are interested.
>
> I'm not sure I see how that's a solution..?  Wouldn't that mean that a
> SIGHUP with an encrypted key would result in a failure?
>
> The solution, at least in my view, seems to be to say "sorry, we can't
> reload the SSL stuff if you used a passphrase to unlock the key on
> startup, you will have to perform a restart if you want the SSL bits to
> be changed."

Sorry, I was very unclear. I meant refusing the reload the SSL context 
if there is a pass phrase, but that the rest of the config will be 
reloaded just fine. This will lead to some log spam on every SIGHUP for 
people with a pass phrase but should otherwise work as before.

Andreas

  62. Re: [PATCH] Reload SSL certificates on SIGHUP

    Stephen Frost <sfrost@snowman.net> — 2017-01-04T15:18:50Z

    * Andreas Karlsson (andreas@proxel.se) wrote:
> On 01/04/2017 04:14 PM, Stephen Frost wrote:
> >* Andreas Karlsson (andreas@proxel.se) wrote:
> >>A possible solution might be to only add the error throwing hook
> >>when loading certificates during SIGHUP (and at Windows) and to work
> >>as before on startup. Would that be an acceptable solution? I could
> >>write a patch for this if people are interested.
> >
> >I'm not sure I see how that's a solution..?  Wouldn't that mean that a
> >SIGHUP with an encrypted key would result in a failure?
> >
> >The solution, at least in my view, seems to be to say "sorry, we can't
> >reload the SSL stuff if you used a passphrase to unlock the key on
> >startup, you will have to perform a restart if you want the SSL bits to
> >be changed."
> 
> Sorry, I was very unclear. I meant refusing the reload the SSL
> context if there is a pass phrase, but that the rest of the config
> will be reloaded just fine. This will lead to some log spam on every
> SIGHUP for people with a pass phrase but should otherwise work as
> before.

Right, that sounds like it'd work for me, at least.

Thanks!

Stephen

  63. Re: [PATCH] Reload SSL certificates on SIGHUP

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-04T15:26:19Z

    Andreas Karlsson <andreas@proxel.se> writes:
> Sorry, I was very unclear. I meant refusing the reload the SSL context 
> if there is a pass phrase, but that the rest of the config will be 
> reloaded just fine. This will lead to some log spam on every SIGHUP for 
> people with a pass phrase but should otherwise work as before.

How will you know whether there's a pass phrase?

			regards, tom lane

  64. Re: [PATCH] Reload SSL certificates on SIGHUP

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-01-04T15:46:28Z

    On 1/4/17 10:26 AM, Tom Lane wrote:
> Andreas Karlsson <andreas@proxel.se> writes:
>> Sorry, I was very unclear. I meant refusing the reload the SSL context 
>> if there is a pass phrase, but that the rest of the config will be 
>> reloaded just fine. This will lead to some log spam on every SIGHUP for 
>> people with a pass phrase but should otherwise work as before.
> 
> How will you know whether there's a pass phrase?

One could register a password callback that remembers whether it was called.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  65. Re: [PATCH] Reload SSL certificates on SIGHUP

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-04T15:57:20Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 1/4/17 10:26 AM, Tom Lane wrote:
>> How will you know whether there's a pass phrase?

> One could register a password callback that remembers whether it was called.

Hmm ... actually, we don't even need to work that hard.  If we simply
use the callback that's there now, but only during reloads not server
start, then we get the desired behavior.  Reloads will fail because
the wrong passphrase was returned by the callback, and we'll keep the
current SSL state.  It would probably be worth tweaking things to minimize
the amount of log spam that you get from that; but it would work, for
values of "work" similar to what was there before.

I still maintain that the existing solution for passphrases is useless,
but in the interest of removing objections to the current patch, I'll
go make that happen.

			regards, tom lane

  66. Re: [PATCH] Reload SSL certificates on SIGHUP

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-01-04T16:37:54Z

    On 1/4/17 10:57 AM, Tom Lane wrote:
> I still maintain that the existing solution for passphrases is useless,
> but in the interest of removing objections to the current patch, I'll
> go make that happen.

Sounds good.

Looking around briefly (e.g., Apache, nginx), the standard approach
appears to be a configuration setting that gets the password from an
external program or file.  (Although the default still appears to be to
get from tty.)

systemd has support for getting passwords to services without tty.

So if someone is interested, there is some room for enhancement here.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  67. Re: [PATCH] Reload SSL certificates on SIGHUP

    Stephen Frost <sfrost@snowman.net> — 2017-01-04T16:49:58Z

    * Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:
> On 1/4/17 10:57 AM, Tom Lane wrote:
> > I still maintain that the existing solution for passphrases is useless,
> > but in the interest of removing objections to the current patch, I'll
> > go make that happen.
> 
> Sounds good.

Agreed, thanks.

> Looking around briefly (e.g., Apache, nginx), the standard approach
> appears to be a configuration setting that gets the password from an
> external program or file.  (Although the default still appears to be to
> get from tty.)

Right, the MIT Kerberos daemon will definitely prompt for the passphrase
for the master key on the terminal also.  They might also have a way to
get it from a program now, not sure, it's been a while, but it was a
requirement from NIST 800-53 to not have unencrypted keys on the
filesystem and I had to address that for the MIT Kerberos master key and
the private keys for various SSL-using services.

> systemd has support for getting passwords to services without tty.

Oh, that's interesting, I wasn't aware of that.

> So if someone is interested, there is some room for enhancement here.

Agreed.

Thanks!

Stephen

  68. Re: [PATCH] Reload SSL certificates on SIGHUP

    Robert Haas <robertmhaas@gmail.com> — 2017-01-05T16:15:24Z

    On Wed, Jan 4, 2017 at 11:49 AM, Stephen Frost <sfrost@snowman.net> wrote:
>> systemd has support for getting passwords to services without tty.
>
> Oh, that's interesting, I wasn't aware of that.
>
>> So if someone is interested, there is some room for enhancement here.
>
> Agreed.

The first thing that pops into my head is that we could add a GUC
ssl_cert_passphrase_command whose contents get executed as a shell
command when we need a passphrase; that program is expected to emit
the passphrase and nothing else on standard output and then exit(0).
Blah blah logging blah blah failure handling.  That's not trivial to
implement if you want the postmaster to still be responsive while the
command is running, but I think it could be done.  (I'm not
volunteering.)

Of course, if there's some sort of commonly-used library out there for
this sort of thing where we can just link against it and call whatever
APIs it exposes, that might be a better alternative, or something to
support in addition, but I don't really know whether there's any
standardization in this area.

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

  69. Re: [PATCH] Reload SSL certificates on SIGHUP

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-05T16:55:16Z

    Robert Haas <robertmhaas@gmail.com> writes:
> Of course, if there's some sort of commonly-used library out there for
> this sort of thing where we can just link against it and call whatever
> APIs it exposes, that might be a better alternative, or something to
> support in addition, but I don't really know whether there's any
> standardization in this area.

I was wondering if we could make use of ssh-agent.  But it seems to want
to hold the private key itself, so that you have to communicate with it
every time you need an operation done with the key.  I'm not sure what the
performance of that is like, and I am sure that the code would look a
whole lot different from the path where we hold the key locally.  It might
be workable if OpenSSL already incorporates library routines for talking
to ssh-agent, but I haven't looked.

			regards, tom lane