pgsql: Provide a TLS init hook

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

pgsql: Provide a TLS init hook

Andrew Dunstan
Provide a TLS init hook

The default hook function sets the default password callback function.
In order to allow preloaded libraries to have an opportunity to override
the default, TLS initialization if now delayed slightly until after
shared preloaded libraries have been loaded.

A test module is provided which contains a trivial example that decodes
an obfuscated password for an SSL certificate.

Author: Andrew Dunstan
Reviewed By: Andreas Karlsson, Asaba Takanori
Discussion: https://postgr.es/m/04116472-818b-5859-1d74-3d995aab2252@...

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/896fcdb230e729652d37270c8606ccdc45212f0d

Modified Files
--------------
src/backend/libpq/be-secure-openssl.c              | 48 +++++++-----
src/backend/postmaster/postmaster.c                | 22 +++---
src/include/libpq/libpq-be.h                       |  4 +
src/test/modules/Makefile                          |  5 ++
.../modules/ssl_passphrase_callback/.gitignore     |  1 +
src/test/modules/ssl_passphrase_callback/Makefile  | 24 ++++++
.../modules/ssl_passphrase_callback/server.crt     | 19 +++++
.../modules/ssl_passphrase_callback/server.key     | 30 ++++++++
.../ssl_passphrase_callback/ssl_passphrase_func.c  | 88 ++++++++++++++++++++++
.../ssl_passphrase_callback/t/001_testfunc.pl      | 80 ++++++++++++++++++++
src/tools/msvc/Mkvcbuild.pm                        |  2 +-
11 files changed, 292 insertions(+), 31 deletions(-)

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Provide a TLS init hook

Tom Lane-2
Andrew Dunstan <[hidden email]> writes:
> Provide a TLS init hook

Buildfarm's not terribly happy --- I suspect that the makefile for
the new test module is failing to link in libopenssl explicitly.
Some platforms are more forgiving of that than others.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Provide a TLS init hook

Tom Lane-2
I wrote:
> Buildfarm's not terribly happy --- I suspect that the makefile for
> the new test module is failing to link in libopenssl explicitly.

Concretely, I see that contrib/sslinfo has

SHLIB_LINK += $(filter -lssl -lcrypto -lssleay32 -leay32, $(LIBS))

which you probably need to crib here.  There might be some analogous
magic in the MSVC files, too.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Provide a TLS init hook

Tom Lane-2
I wrote:
> Concretely, I see that contrib/sslinfo has
> SHLIB_LINK += $(filter -lssl -lcrypto -lssleay32 -leay32, $(LIBS))

I verified that that fixes things on macOS and pushed it, along with
a couple other minor fixes.

However, I'm quite desperately unhappy that the new test module
does this:

$node->append_conf('postgresql.conf', "listen_addresses = 'localhost'");

That's opening a security hole.  Note that we do *not* run src/test/ssl
by default, and it has a README warning people not to run it on multiuser
systems.  It seems 100% unacceptable for this test to fire up a similarly
insecure server without so much as a by-your-leave.

I don't actually see why we need the localhost port at all --- it doesn't
look like this test ever attempts to connect to the server.  So couldn't
we just drop that?

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Provide a TLS init hook

Andrew Dunstan-8

On 3/25/20 7:44 PM, Tom Lane wrote:
> I wrote:
>> Concretely, I see that contrib/sslinfo has
>> SHLIB_LINK += $(filter -lssl -lcrypto -lssleay32 -leay32, $(LIBS))
> I verified that that fixes things on macOS and pushed it, along with
> a couple other minor fixes.


Thanks.


>
> However, I'm quite desperately unhappy that the new test module
> does this:
>
> $node->append_conf('postgresql.conf', "listen_addresses = 'localhost'");
>
> That's opening a security hole.  Note that we do *not* run src/test/ssl
> by default, and it has a README warning people not to run it on multiuser
> systems.  It seems 100% unacceptable for this test to fire up a similarly
> insecure server without so much as a by-your-leave.
>
> I don't actually see why we need the localhost port at all --- it doesn't
> look like this test ever attempts to connect to the server.  So couldn't
> we just drop that?
>
>



Seems reasonable. I just tested that and it seems quite happy, so I'll
make the change.


cheers


andrew


--
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Provide a TLS init hook

Tom Lane-2
Andrew Dunstan <[hidden email]> writes:
> On 3/25/20 7:44 PM, Tom Lane wrote:
>> I don't actually see why we need the localhost port at all --- it doesn't
>> look like this test ever attempts to connect to the server.  So couldn't
>> we just drop that?

> Seems reasonable. I just tested that and it seems quite happy, so I'll
> make the change.

Cool, thanks.

jacana has just exposed a different problem: it's not configured
--with-openssl, but the buildfarm script is trying to run this
new test module anyway.  I'm confused about the reason.
"make installcheck" in src/test/modules does the right thing,
but seemingly that client is doing something different?

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Provide a TLS init hook

Andrew Dunstan-8

On 3/25/20 9:28 PM, Tom Lane wrote:

> Andrew Dunstan <[hidden email]> writes:
>> On 3/25/20 7:44 PM, Tom Lane wrote:
>>> I don't actually see why we need the localhost port at all --- it doesn't
>>> look like this test ever attempts to connect to the server.  So couldn't
>>> we just drop that?
>> Seems reasonable. I just tested that and it seems quite happy, so I'll
>> make the change.
> Cool, thanks.
>
> jacana has just exposed a different problem: it's not configured
> --with-openssl, but the buildfarm script is trying to run this
> new test module anyway.  I'm confused about the reason.
> "make installcheck" in src/test/modules does the right thing,
> but seemingly that client is doing something different?
>
>



Ugh. I have put in place a hack to clear the error on jacana. Yes, the
client does something different so we can run each module separately.
Trawling through the output and files for one test on its own is hard
enough, I don't want to aggregate them.


cheers


andrew


--
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Provide a TLS init hook

Tom Lane-2
Andrew Dunstan <[hidden email]> writes:
> On 3/25/20 9:28 PM, Tom Lane wrote:
>> jacana has just exposed a different problem: it's not configured
>> --with-openssl, but the buildfarm script is trying to run this
>> new test module anyway.  I'm confused about the reason.
>> "make installcheck" in src/test/modules does the right thing,
>> but seemingly that client is doing something different?

> Ugh. I have put in place a hack to clear the error on jacana. Yes, the
> client does something different so we can run each module separately.
> Trawling through the output and files for one test on its own is hard
> enough, I don't want to aggregate them.

Well, I'm confused, because my own critters are running this as part
of a single make-installcheck-in-src/test/modules step, eg

https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=longfin&dt=2020-03-26%2002%3A09%3A08&stg=testmodules-install-check-C

Why is jacana doing it differently?

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Provide a TLS init hook

Andrew Dunstan-8

On 3/26/20 9:50 AM, Tom Lane wrote:

> Andrew Dunstan <[hidden email]> writes:
>> On 3/25/20 9:28 PM, Tom Lane wrote:
>>> jacana has just exposed a different problem: it's not configured
>>> --with-openssl, but the buildfarm script is trying to run this
>>> new test module anyway.  I'm confused about the reason.
>>> "make installcheck" in src/test/modules does the right thing,
>>> but seemingly that client is doing something different?
>> Ugh. I have put in place a hack to clear the error on jacana. Yes, the
>> client does something different so we can run each module separately.
>> Trawling through the output and files for one test on its own is hard
>> enough, I don't want to aggregate them.
> Well, I'm confused, because my own critters are running this as part
> of a single make-installcheck-in-src/test/modules step, eg
>
> https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=longfin&dt=2020-03-26%2002%3A09%3A08&stg=testmodules-install-check-C
>
> Why is jacana doing it differently?



longfin is also running it (first) here
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=longfin&dt=2020-03-26%2014%3A39%3A51&stg=ssl_passphrase_callback-check


That's where jacana failed.


I don't think this belongs in installcheck, we should add
'NO_INSTALLCHECK = 1' to the Makefile.


cheers


andrew

--
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Provide a TLS init hook

Tom Lane-2
Andrew Dunstan <[hidden email]> writes:
> On 3/26/20 9:50 AM, Tom Lane wrote:
>> Why is jacana doing it differently?

> longfin is also running it (first) here
> https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=longfin&dt=2020-03-26%2014%3A39%3A51&stg=ssl_passphrase_callback-check

Oh, I missed that.  Isn't that pretty duplicative of the
testmodules-install phase?

> I don't think this belongs in installcheck, we should add
> 'NO_INSTALLCHECK = 1' to the Makefile.

Why?  The other src/test/modules/ modules with TAP tests do not
specify that, with the exception of commit_ts which has a solid
doesnt-work-in-the-default-configuration excuse.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Provide a TLS init hook

Andrew Dunstan-8

On 3/26/20 11:31 AM, Tom Lane wrote:
> Andrew Dunstan <[hidden email]> writes:
>> On 3/26/20 9:50 AM, Tom Lane wrote:
>>> Why is jacana doing it differently?
>> longfin is also running it (first) here
>> https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=longfin&dt=2020-03-26%2014%3A39%3A51&stg=ssl_passphrase_callback-check
> Oh, I missed that.  Isn't that pretty duplicative of the
> testmodules-install phase?


Yes, but see below


>
>> I don't think this belongs in installcheck, we should add
>> 'NO_INSTALLCHECK = 1' to the Makefile.
> Why?  The other src/test/modules/ modules with TAP tests do not
> specify that, with the exception of commit_ts which has a solid
> doesnt-work-in-the-default-configuration excuse.
>
>



That seems wrong, installcheck should be testing against an installed
instance, and the TAP tests don't. Moreover, from the buildfarm's POV
it's completely wrong, as we call the installcheck targets multiple
times, once for each configured locale. See one of the animals that
tests multiple locales (e.g. crake or prion)


src/test is a mess, TBH, and I have spent quite some time trying to get
it so that we test everything but without duplication, clearly without
complete success.


cheers


andrew



--
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Provide a TLS init hook

Tom Lane-2
Andrew Dunstan <[hidden email]> writes:
> On 3/26/20 11:31 AM, Tom Lane wrote:
>> Andrew Dunstan <[hidden email]> writes:
>>> I don't think this belongs in installcheck, we should add
>>> 'NO_INSTALLCHECK = 1' to the Makefile.

>> Why?  The other src/test/modules/ modules with TAP tests do not
>> specify that, with the exception of commit_ts which has a solid
>> doesnt-work-in-the-default-configuration excuse.

> That seems wrong, installcheck should be testing against an installed
> instance, and the TAP tests don't.

So?  We clearly document that for the TAP tests, "make installcheck"
means "use the installed executables, but run a new instance" [1].

> Moreover, from the buildfarm's POV
> it's completely wrong, as we call the installcheck targets multiple
> times, once for each configured locale. See one of the animals that
> tests multiple locales (e.g. crake or prion)

Yeah.  That's productive if you think the tests might be
locale-sensitive.  I doubt that any of the ones under src/test/modules/
actually are at the moment, so maybe this is a waste of buildfarm effort.
But I don't think that it's the place of the Makefiles to dictate such
policy, and especially not for them to do so by breaking the ability to
use "make installcheck" at all.

                        regards, tom lane

[1] https://www.postgresql.org/docs/devel/regress-tap.html