Обсуждение: Re: pgsql: Implement channel binding tls-server-end-point for SCRAM

On Thu, Jan 4, 2018 at 4:09 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Fri, Jan 5, 2018 at 9:36 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
>> Implement channel binding tls-server-end-point for SCRAM
>
> FYI some BF animals are saying:
>
> libpq/be-secure-openssl.o: In function `be_tls_get_certificate_hash':
> /home/pgbuildfarm/buildroot-termite/HEAD/pgsql.build/../pgsql/src/backend/libpq/be-secure-openssl.c:1268:
> undefined reference to `X509_get_signature_nid'

The SSL tests on chipmunk failed in the last run.  I assume that's
probably the fault of this patch, or one of the follow-on commits:

# Running: psql -X -A -t -c SELECT 'connected with user=ssltestuser
dbname=trustdb sslmode=require hostaddr=127.0.0.1
scram_channel_binding=tls-server-end-point' -d user=ssltestuser
dbname=trustdb sslmode=require hostaddr=127.0.0.1
scram_channel_binding=tls-server-end-point
psql: channel binding type "tls-server-end-point" is not supported by this build
not ok 4 - SCRAM authentication with tls-server-end-point as channel binding

#   Failed test 'SCRAM authentication with tls-server-end-point as
channel binding'
#   at /home/pgbfarm/buildroot/HEAD/pgsql.build/src/test/ssl/ServerSetup.pm
line 64.
# Running: psql -X -A -t -c SELECT 'connected with user=ssltestuser
dbname=trustdb sslmode=require hostaddr=127.0.0.1
scram_channel_binding=not-exists' -d user=ssltestuser dbname=trustdb
sslmode=require hostaddr=127.0.0.1 scram_channel_binding=not-exists
psql: FATAL:  unsupported SCRAM channel-binding type
ok 5 - SCRAM authentication with invalid channel binding
### Stopping node "master" using mode immediate
# Running: pg_ctl -D
/home/pgbfarm/buildroot/HEAD/pgsql.build/src/test/ssl/tmp_check/t_002_scram_master_data/pgdata
-m immediate stop
waiting for server to shut down.... done
server stopped
# No postmaster PID for node "master"
# Looks like you failed 1 test of 5.

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

On Fri, Jan 5, 2018 at 10:47 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> The SSL tests on chipmunk failed in the last run.  I assume that's
> probably the fault of this patch, or one of the follow-on commits:

Thanks for the heads-up, Robert. I did not notice the failure. That's
the fault of 054e8c6c. Raspbian is using OpenSSL 1.0.1t (package list
can be downloaded in
http://archive.raspbian.org/raspbian/dists/wheezy/main/binary-armhf/Packages
for 38MB), which does not have the necessary facilities to implement
tls-server-end-point as upstream has added necessary APIs only in
1.0.2.

In order to do things cleanly, we should make this TAP test
conditional on the version of OpenSSL. There have been discussions in
the past to make a module dedicated to that, but no clear patch or
approach has showed up. This can be retrieved with SSLeay_version() or
"openssl version", but that seems not fun nor stable to rely on
openssl to be in PATH. I don't see disabling this test helping either,
but we could consider that without an appropriate module to track
dependencies in a build with its versions. I would be personally fine
with having an environment variable switch I could use to enable the
test as well as I use already a script to run all regression tests in
the tree (src/test/ssl is not run by default as it is unsecure for
shared environments, without counting on meltdowns).

Thoughts from others?
-- 
Michael

On 1/5/18 09:28, Michael Paquier wrote:
> In order to do things cleanly, we should make this TAP test
> conditional on the version of OpenSSL.

How about looking into pg_config.h, like in the attached patch?

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

Peter Eisentraut wrote:
> On 1/5/18 09:28, Michael Paquier wrote:
> > In order to do things cleanly, we should make this TAP test
> > conditional on the version of OpenSSL.
> 
> How about looking into pg_config.h, like in the attached patch?

I suppose if this starts to spread, we'll come up with a better approach
... maybe generating a Perl file with variables that can be slurped more
ellegantly, or something like that.  (We mentioned the need for
config-based test selection re. patches for new SSL implementations.)

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

On Sat, Jan 6, 2018 at 2:56 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Peter Eisentraut wrote:
>> On 1/5/18 09:28, Michael Paquier wrote:
>> > In order to do things cleanly, we should make this TAP test
>> > conditional on the version of OpenSSL.
>>
>> How about looking into pg_config.h, like in the attached patch?

+# Determine whether build supports tls-server-end-point.
+open my $pg_config_h, '<', '../../include/pg_config.h' or die "$!";
+my $supports_tls_server_end_point = (grep {/^#define
HAVE_X509_GET_SIGNATURE_NID 1/} <$pg_config_h>);
+close $pg_config_h;

Good idea as a whole, but I don't think this is the right approach. As
we include $(bindir) in PATH when running the prove command, why not
getting the include path from "pg_config --includedir"?

> I suppose if this starts to spread, we'll come up with a better approach
> ... maybe generating a Perl file with variables that can be slurped more
> ellegantly, or something like that.  (We mentioned the need for
> config-based test selection re. patches for new SSL implementations.)

One case I have in mind where this would be nice to have is
020_pg_receivewal.pl to have tests depending on if PG is built with
zlib or not. So we definitely want something more. At least I do. I
agree that the most elegant approach would be to generate pg_config.h
from this variable set, and not feed on parsing pg_config.h directly.
Or we could just live with an API in TestLib.pm which is able to get
the wanted information as Peter is doing but for a wanted variable
from pg_config. I could use that for my test case with HAVE_LIBZ as
well.
--
Michael

On Sat, Jan 06, 2018 at 09:10:51AM +0900, Michael Paquier wrote:
> On Sat, Jan 6, 2018 at 2:56 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Good idea as a whole, but I don't think this is the right approach. As
> we include $(bindir) in PATH when running the prove command, why not
> getting the include path from "pg_config --includedir"?

So, I have been looking at that, and I propose the following
counter-patch which implements this idea by adding a new routine as
TestLib::config_check which is able to check within pg_config.h if a
given regexp matches or not for the installation on which TAP tests are
being run. I have tested with Postgres compiled with both OpenSSL 1.0.1
and 1.0.2, in which case the connection test respectively fails and
passes, causing the test to be correctly handled. This is based on
Peter's patch upthread.
--
Michael

On 1/8/18 08:14, Michael Paquier wrote:
> On Sat, Jan 06, 2018 at 09:10:51AM +0900, Michael Paquier wrote:
>> On Sat, Jan 6, 2018 at 2:56 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>> Good idea as a whole, but I don't think this is the right approach. As
>> we include $(bindir) in PATH when running the prove command, why not
>> getting the include path from "pg_config --includedir"?
> 
> So, I have been looking at that, and I propose the following
> counter-patch which implements this idea by adding a new routine as
> TestLib::config_check which is able to check within pg_config.h if a
> given regexp matches or not for the installation on which TAP tests are
> being run. I have tested with Postgres compiled with both OpenSSL 1.0.1
> and 1.0.2, in which case the connection test respectively fails and
> passes, causing the test to be correctly handled. This is based on
> Peter's patch upthread.

Committed.  I like counter-patches.

(I renamed the function a bit to check_pg_config, to make the naming
similar to other functions in TestLib.)

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

On Tue, Jan 09, 2018 at 12:49:11PM -0500, Peter Eisentraut wrote:
> Committed.  I like counter-patches.
>
> (I renamed the function a bit to check_pg_config, to make the naming
> similar to other functions in TestLib.)

Thanks for committing. I think that you should have authorship as well,
which is something that c3d41ccf does not mention directly.
--
Michael