Thread: 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
Attachment
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
Attachment
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