Обсуждение: pgsql: Provide a TLS init hook
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@2ndQuadrant.com 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(-)
Andrew Dunstan <andrew@dunslane.net> 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
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
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
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
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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
On 3/25/20 9:28 PM, Tom Lane wrote: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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
On 3/26/20 9:50 AM, Tom Lane wrote: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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
On 3/26/20 11:31 AM, Tom Lane wrote: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 3/26/20 11:31 AM, Tom Lane wrote:
>> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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