pgsql: Implement channel binding tls-server-end-point for SCRAM
Implement channel binding tls-server-end-point for SCRAM
This adds a second standard channel binding type for SCRAM. It is
mainly intended for third-party clients that cannot implement
tls-unique, for example JDBC.
Author: Michael Paquier <michael.paquier@gmail.com>
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/d3fb72ea6de58d285e278459bca9d7cdf7f6a38b
Modified Files
--------------
doc/src/sgml/protocol.sgml | 17 ++++---
src/backend/libpq/auth-scram.c | 20 ++++++--
src/backend/libpq/be-secure-openssl.c | 61 ++++++++++++++++++++++++
src/include/common/scram-common.h | 1 +
src/include/libpq/libpq-be.h | 1 +
src/interfaces/libpq/fe-auth-scram.c | 15 ++++++
src/interfaces/libpq/fe-secure-openssl.c | 80 ++++++++++++++++++++++++++++++++
src/interfaces/libpq/libpq-int.h | 1 +
src/test/ssl/t/002_scram.pl | 5 +-
9 files changed, 189 insertions(+), 12 deletions(-)
Peter Eisentraut <peter_e@gmx.net> writes:
Implement channel binding tls-server-end-point for SCRAM
Buildfarm doesn't like this one bit.
regards, tom lane
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'
--
Thomas Munro
http://www.enterprisedb.com
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
Attachments:
0001-Fix-ssl-tests-for-when-tls-server-end-point-is-not-s.patchtext/plain; charset=UTF-8; name=0001-Fix-ssl-tests-for-when-tls-server-end-point-is-not-s.patch; x-mac-creator=0; x-mac-type=0Download
From d1c309a4c07c47f06650fd8231938e1eaed1342e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
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
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
Attachments:
fix_ssl_tests_scram_v2.patchtext/plain; charset=us-asciiDownload
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");
On 4 January 2018 at 21:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
Implement channel binding tls-server-end-point for SCRAM
Buildfarm doesn't like this one bit.
Can't we automate these messages? Seems strange to send manual emails
every time. We do know who the commits are coming from and we have
their email address.
It would be useful to get automatic message giving a summary of
buildfarm results at 15, 30 and 60 minute intervals, even if it is
just ALL CLEAR.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Simon Riggs <simon@2ndquadrant.com> writes:
On 4 January 2018 at 21:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
Implement channel binding tls-server-end-point for SCRAM
Buildfarm doesn't like this one bit.
Can't we automate these messages?
It's not that easy. First, the buildfarm gets random failures all the
time, due to this and that. Second, if several commits have occurred
since the critter's last run, it requires some human judgment to figure
out which commit is probably to blame.
You could ameliorate the first problem by waiting for multiple failures
to show up ... but the longer you wait, the worse the second problem
becomes (and the less useful the report would be anyway).
It would be useful to get automatic message giving a summary of
buildfarm results at 15, 30 and 60 minute intervals, even if it is
just ALL CLEAR.
The raw result of that would be too noisy to be useful. I've wondered
about getting the buildfarm status page to filter out the more obvious
classes of "random failure" --- git pull failures would be one, and
another would be if "no space left on device" appears anywhere in any
of the report's log files. Don't know how far that would get us, though.
regards, tom lane
On 01/08/2018 11:01 AM, Tom Lane wrote:
Simon Riggs <simon@2ndquadrant.com> writes:
On 4 January 2018 at 21:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
Implement channel binding tls-server-end-point for SCRAM
Buildfarm doesn't like this one bit.
Can't we automate these messages?
It's not that easy. First, the buildfarm gets random failures all the
time, due to this and that. Second, if several commits have occurred
since the critter's last run, it requires some human judgment to figure
out which commit is probably to blame.You could ameliorate the first problem by waiting for multiple failures
to show up ... but the longer you wait, the worse the second problem
becomes (and the less useful the report would be anyway).It would be useful to get automatic message giving a summary of
buildfarm results at 15, 30 and 60 minute intervals, even if it is
just ALL CLEAR.The raw result of that would be too noisy to be useful. I've wondered
about getting the buildfarm status page to filter out the more obvious
classes of "random failure" --- git pull failures would be one, and
another would be if "no space left on device" appears anywhere in any
of the report's log files. Don't know how far that would get us, though.
Without triangulating via something like git-bisect I suspect we'd very
soon find any automated system very tiresome indeed.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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