Re: pgsql: Implement channel binding tls-server-end-point for SCRAM

[Peter Eisentraut]

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

Re: pgsql: Implement channel binding tls-server-end-point for SCRAM

[Michael Paquier]

On Sat, Jan 06, 2018 at 09:10:51AM +0900, Michael Paquier wrote:
> On Sat, Jan 6, 2018 at 2:56 AM, Alvaro Herrera  
> 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
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 72826d5bad..c49b4336c1 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -26,6 +26,7 @@ our @EXPORT = qw(
   slurp_dir
   slurp_file
   append_to_file
+  config_check
   system_or_bail
   system_log
   run_log
@@ -221,6 +222,27 @@ sub append_to_file
close $fh;
 }
 
+# Check presence of a given regexp within pg_config.h for the installation
+# where tests are running, returning a match status result depending on
+# that.
+sub config_check
+{
+   my ($regexp) = @_;
+   my ($stdout, $stderr);
+   my $result = IPC::Run::run [ 'pg_config', '--includedir' ], '>',
+   \$stdout, '2>', \$stderr;
+
+   print("# Checking for match with expression \"$regexp\" in 
pg_config.h\n");
+
+   die "could not execute pg_config" if $result != 1;
+   chomp($stdout);
+
+   open my $pg_config_h, '<', "$stdout/pg_config.h" or die "$!";
+   my $match = (grep {/^$regexp/} <$pg_config_h>);
+   close $pg_config_h;
+   return $match;
+}
+
 #
 # Test functions
 #
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index 3f425e00f0..73c8f7adf2 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -11,6 +11,10 @@ use File::Copy;
 # This is the hostname used to connect to the server.
 my $SERVERHOSTADDR = '127.0.0.1';
 
+# Determine whether build supports tls-server-end-point.
+my $supports_tls_server_end_point =
+   TestLib::config_check("#define HAVE_X509_GET_SIGNATURE_NID 1");
+
 # Allocation of base connection string shared among multiple tests.
 my $common_connstr;
 
@@ -44,10 +48,19 @@ test_connect_ok($common_connstr,
"SCRAM authentication with tls-unique as channel binding");
 test_connect_ok($common_connstr,
"scram_channel_binding=''",
-   "SCRAM authentication without channel binding");
-test_connect_ok($common_connstr,
-   "scram_channel_binding=tls-server-end-point",
-   "SCRAM authentication with tls-server-end-point as channel binding");
+   "SCRAM authentication without channel binding");
+if ($supports_tls_server_end_point)
+{
+   test_connect_ok($common_connstr,
+   
"scram_channel_binding=tls-server-end-point",
+   "SCRAM authentication with 
tls-server-end-point as channel binding");
+}
+else
+{
+   test_connect_fails($common_connstr,
+   
"scram_channel_binding=tls-server-end-point",
+   "SCRAM authentication with 
tls-server-end-point as channel binding");
+}
 test_connect_fails($common_connstr,
"scram_channel_binding=not-exists",
"SCRAM authentication with invalid channel binding");


signature.asc
Description: PGP signature

Re: pgsql: Implement channel binding tls-server-end-point for SCRAM

[Michael Paquier]

On Sat, Jan 6, 2018 at 2:56 AM, Alvaro Herrera  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

Re: pgsql: Implement channel binding tls-server-end-point for SCRAM

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: pgsql: Implement channel binding tls-server-end-point for SCRAM

[Peter Eisentraut]

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
From d1c309a4c07c47f06650fd8231938e1eaed1342e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 5 Jan 2018 09:55:01 -0500
Subject: [PATCH] Fix ssl tests for when tls-server-end-point is not supported

---
 src/test/ssl/t/002_scram.pl | 22 ++
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index 3f425e00f0..92b84e1565 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -11,6 +11,11 @@
 # This is the hostname used to connect to the server.
 my $SERVERHOSTADDR = '127.0.0.1';
 
+# 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;
+
 # Allocation of base connection string shared among multiple tests.
 my $common_connstr;
 
@@ -44,10 +49,19 @@
"SCRAM authentication with tls-unique as channel binding");
 test_connect_ok($common_connstr,
"scram_channel_binding=''",
-   "SCRAM authentication without channel binding");
-test_connect_ok($common_connstr,
-   "scram_channel_binding=tls-server-end-point",
-   "SCRAM authentication with tls-server-end-point as channel binding");
+   "SCRAM authentication without channel binding");
+if ($supports_tls_server_end_point)
+{
+   test_connect_ok($common_connstr,
+   
"scram_channel_binding=tls-server-end-point",
+   "SCRAM authentication with 
tls-server-end-point as channel binding");
+}
+else
+{
+   test_connect_fails($common_connstr,
+   
"scram_channel_binding=tls-server-end-point",
+   "SCRAM authentication with 
tls-server-end-point as channel binding");
+}
 test_connect_fails($common_connstr,
"scram_channel_binding=not-exists",
"SCRAM authentication with invalid channel binding");
-- 
2.15.1

Re: pgsql: Implement channel binding tls-server-end-point for SCRAM

[Robert Haas]

On Thu, Jan 4, 2018 at 4:09 PM, Thomas Munro
 wrote:
> On Fri, Jan 5, 2018 at 9:36 AM, Peter Eisentraut  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