Re: [HACKERS] src/test/ssl broken on HEAD
On Tue, Sep 15, 2015 at 5:23 PM, Peter Eisentraut wrote: > The only trick, as I remember, was that clients tend to prefer SSL > automatically, which we probably don't want for Unix-domain sockets, so > we'd need to tweak those settings a bit. > > The "old patch" referred to in that old thread wasn't actually attached, > so here it is, for amusement. Wouldn't this need as well a separate configuration value in pg_hba.conf? Let's say localssl. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/test/ssl broken on HEAD
On 9/2/15 7:15 PM, Andres Freund wrote: >> Add a regression test suite for SSL support. >> >> It's not run by the global "check" or "installcheck" targets, because the >> temporary installation it creates accepts TCP connections from any user >> the same host, which is insecure. > > We could just implement SSL over unix sockets. Obviously the > connection-encryption aspect isn't actually useful, but e.g. client > certs still make sense. Besides, it allows to avoid concerns like the > above... See old discussion here: http://www.postgresql.org/message-id/49ca2524.5010...@gmx.net At the time, we didn't have this test suite, obviously, so the utility would be have been limited, but now it looks quite interesting. The only trick, as I remember, was that clients tend to prefer SSL automatically, which we probably don't want for Unix-domain sockets, so we'd need to tweak those settings a bit. The "old patch" referred to in that old thread wasn't actually attached, so here it is, for amusement. diff -ur ../cvs-pgsql/src/backend/postmaster/postmaster.c ./src/backend/postmaster/postmaster.c --- ../cvs-pgsql/src/backend/postmaster/postmaster.c 2008-01-04 15:55:25.0 +0100 +++ ./src/backend/postmaster/postmaster.c 2008-01-04 16:44:35.0 +0100 @@ -1448,8 +1448,8 @@ char SSLok; #ifdef USE_SSL - /* No SSL when disabled or on Unix sockets */ - if (!EnableSSL || IS_AF_UNIX(port->laddr.addr.ss_family)) + /* No SSL when disabled */ + if (!EnableSSL) SSLok = 'N'; else SSLok = 'S'; /* Support for SSL */ diff -ur ../cvs-pgsql/src/interfaces/libpq/fe-connect.c ./src/interfaces/libpq/fe-connect.c --- ../cvs-pgsql/src/interfaces/libpq/fe-connect.c 2008-01-04 15:55:31.0 +0100 +++ ./src/interfaces/libpq/fe-connect.c 2008-01-04 16:51:09.0 +0100 @@ -1261,11 +1261,6 @@ * If SSL is enabled and we haven't already got it running, * request it instead of sending the startup message. */ -if (IS_AF_UNIX(conn->raddr.addr.ss_family)) -{ - /* Don't bother requesting SSL over a Unix socket */ - conn->allow_ssl_try = false; -} if (conn->allow_ssl_try && !conn->wait_ssl_try && conn->ssl == NULL) { -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/test/ssl broken on HEAD
On Thu, Sep 03, 2015 at 01:15:47AM +0200, Andres Freund wrote: > On 2015-09-02 17:03:46 -0400, Robert Haas wrote: > > On Wed, Sep 2, 2015 at 4:50 PM, Andrew Dunstanwrote: > > > Tell me what's needed and I'll look at creating a buildfarm test module > > > for > > > it. > > It's not run by the global "check" or "installcheck" targets, because > > the > > temporary installation it creates accepts TCP connections from any user > > the same host, which is insecure. The buildfarm client may as well ignore that security hazard, because today's buildfarm client starts likewise-exposed postmasters directly. > We could just implement SSL over unix sockets. Obviously the > connection-encryption aspect isn't actually useful, but e.g. client > certs still make sense. Besides, it allows to avoid concerns like the > above... Agreed. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/test/ssl broken on HEAD
On Wed, Aug 26, 2015 at 3:37 AM, Michael Paquierwrote: > On Wed, Aug 26, 2015 at 4:35 PM, Michael Paquier > wrote: >> Only HEAD is impacted, and attached is a patch to fix the problem. > > Actually this version is better, I forgot to update a comment. For so long as this test suite is not run by 'make check-world' or by the buildfarm, it's likely to keep getting broken, and we're likely to keep not noticing. I realize that the decision to exclude this from 'make check-world' was deliberately made for security reasons, but that doesn't take anything away from the maintenance headache. Still, that's not a reason not commit this, so done. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/test/ssl broken on HEAD
On Thu, Sep 3, 2015 at 5:22 AM, Robert Haas wrote: > Still, that's not a reason not commit this, so done. Thanks. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/test/ssl broken on HEAD
On 09/02/2015 04:22 PM, Robert Haas wrote: On Wed, Aug 26, 2015 at 3:37 AM, Michael Paquierwrote: On Wed, Aug 26, 2015 at 4:35 PM, Michael Paquier wrote: Only HEAD is impacted, and attached is a patch to fix the problem. Actually this version is better, I forgot to update a comment. For so long as this test suite is not run by 'make check-world' or by the buildfarm, it's likely to keep getting broken, and we're likely to keep not noticing. I realize that the decision to exclude this from 'make check-world' was deliberately made for security reasons, but that doesn't take anything away from the maintenance headache. Still, that's not a reason not commit this, so done. Tell me what's needed and I'll look at creating a buildfarm test module for it. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/test/ssl broken on HEAD
Andrew Dunstan wrote: > > > On 09/02/2015 04:22 PM, Robert Haas wrote: > >For so long as this test suite is not run by 'make check-world' or by > >the buildfarm, it's likely to keep getting broken, and we're likely to > >keep not noticing. I realize that the decision to exclude this from > >'make check-world' was deliberately made for security reasons, but > >that doesn't take anything away from the maintenance headache. > Tell me what's needed and I'll look at creating a buildfarm test module for > it. AFAICS it just requires running "make check" in src/test/ssl. Maybe this could be part of a generic module that runs "make check" in specified subdirectories; see http://www.postgresql.org/message-id/20150813181107.GC5232@alvherre.pgsql for previous discussion about that. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/test/ssl broken on HEAD
On Wed, Sep 2, 2015 at 4:50 PM, Andrew Dunstanwrote: > Tell me what's needed and I'll look at creating a buildfarm test module for > it. You run the tests via: make -C src/test/ssl check But nota bene security caveats: commit e39250c644ea7cd3904e4e24570db21a209cf97f Author: Heikki Linnakangas Date: Tue Dec 9 17:21:18 2014 +0200 Add a regression test suite for SSL support. It's not run by the global "check" or "installcheck" targets, because the temporary installation it creates accepts TCP connections from any user the same host, which is insecure. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] src/test/ssl broken on HEAD
Hi all, The following commit has broken the SSL test suite (embarrassing and lonely moment): commit 13d856e177e69083f543d6383eeda9e12ce3c55c Author: Heikki Linnakangas heikki.linnakan...@iki.fi Date: Wed Jul 29 19:17:02 2015 +0300 Make TAP tests work on Windows. [...] *Michael Paquier*, reviewed by Noah Misch, some additional tweaking by me. What actually happens is that the SSL suite needs to use a host IP for the tests, but listen_addresses has been reshuffled in the commit above. Only HEAD is impacted, and attached is a patch to fix the problem. I have refactored as well the code so as 127.0.0.1 is not hardcoded around anymore, and the IP used for the tests is centralized. The tests also used a log file, but this does not make sense anymore as all the output from stderr and stdout is captured in a log file. Regards, -- Michael diff --git a/src/test/ssl/ServerSetup.pm b/src/test/ssl/ServerSetup.pm index 8c1b517..79d948a 100644 --- a/src/test/ssl/ServerSetup.pm +++ b/src/test/ssl/ServerSetup.pm @@ -59,7 +59,8 @@ sub chmod_files sub configure_test_server_for_ssl { - my $tempdir = $_[0]; + my $tempdir= $_[0]; + my $serverhost = $_[1]; # Create test users and databases psql 'postgres', CREATE USER ssltestuser; @@ -72,6 +73,7 @@ sub configure_test_server_for_ssl print CONF fsync=off\n; print CONF log_connections=on\n; print CONF log_hostname=on\n; + print CONF listen_addresses='$serverhost'\n; print CONF log_statement=all\n; # enable SSL and set up server key @@ -94,11 +96,11 @@ sub configure_test_server_for_ssl print HBA # TYPE DATABASEUSERADDRESS METHOD\n; print HBA -hostssl trustdb ssltestuser 127.0.0.1/32trust\n; +hostssl trustdb ssltestuser $serverhost/32trust\n; print HBA hostssl trustdb ssltestuser ::1/128 trust\n; print HBA -hostssl certdb ssltestuser 127.0.0.1/32cert\n; +hostssl certdb ssltestuser $serverhost/32cert\n; print HBA hostssl certdb ssltestuser ::1/128 cert\n; close HBA; @@ -121,10 +123,8 @@ sub switch_server_cert print SSLCONF ssl_crl_file='root+client.crl'\n; close SSLCONF; - # Stop and restart server to reload the new config. We cannot use - # restart_test_server() because that overrides listen_addresses to only all - # Unix domain socket connections. - - system_or_bail 'pg_ctl', 'stop', '-D', $tempdir/pgdata; - system_or_bail 'pg_ctl', 'start', '-D', $tempdir/pgdata, '-w'; + # Stop and restart server to reload the new config. We cannot use + # restart_test_server() because that overrides listen_addresses to only all + # Unix domain socket connections. + restart_test_server(); } diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl index 5d24d8d..0d6f339 100644 --- a/src/test/ssl/t/001_ssltests.pl +++ b/src/test/ssl/t/001_ssltests.pl @@ -27,9 +27,6 @@ my $SERVERHOSTADDR = '127.0.0.1'; my $tempdir = TestLib::tempdir; -#my $tempdir = tmp_check; - - # Define a couple of helper functions to test connecting to the server. my $common_connstr; @@ -43,12 +40,7 @@ sub run_test_psql 'psql', '-A', '-t', '-c', SELECT 'connected with $connstr', '-d', $connstr ]; - open CLIENTLOG, $tempdir/client-log - or die Could not open client-log file; - print CLIENTLOG \n# Running test: $connstr $logstring\n; - close CLIENTLOG; - - my $result = run $cmd, '', $tempdir/client-log, '21'; + my $result = run_log($cmd); return $result; } @@ -84,7 +76,7 @@ chmod 0600, ssl/client.key; diag setting up data directory in \$tempdir\...; start_test_server($tempdir); -configure_test_server_for_ssl($tempdir); +configure_test_server_for_ssl($tempdir, $SERVERHOSTADDR); switch_server_cert($tempdir, 'server-cn-only'); ### Part 1. Run client-side tests. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/test/ssl broken on HEAD
On Wed, Aug 26, 2015 at 4:35 PM, Michael Paquier michael.paqu...@gmail.com wrote: Only HEAD is impacted, and attached is a patch to fix the problem. Actually this version is better, I forgot to update a comment. -- Michael diff --git a/src/test/ssl/ServerSetup.pm b/src/test/ssl/ServerSetup.pm index 8c1b517..79d948a 100644 --- a/src/test/ssl/ServerSetup.pm +++ b/src/test/ssl/ServerSetup.pm @@ -59,7 +59,8 @@ sub chmod_files sub configure_test_server_for_ssl { - my $tempdir = $_[0]; + my $tempdir= $_[0]; + my $serverhost = $_[1]; # Create test users and databases psql 'postgres', CREATE USER ssltestuser; @@ -72,6 +73,7 @@ sub configure_test_server_for_ssl print CONF fsync=off\n; print CONF log_connections=on\n; print CONF log_hostname=on\n; + print CONF listen_addresses='$serverhost'\n; print CONF log_statement=all\n; # enable SSL and set up server key @@ -94,11 +96,11 @@ sub configure_test_server_for_ssl print HBA # TYPE DATABASEUSERADDRESS METHOD\n; print HBA -hostssl trustdb ssltestuser 127.0.0.1/32trust\n; +hostssl trustdb ssltestuser $serverhost/32trust\n; print HBA hostssl trustdb ssltestuser ::1/128 trust\n; print HBA -hostssl certdb ssltestuser 127.0.0.1/32cert\n; +hostssl certdb ssltestuser $serverhost/32cert\n; print HBA hostssl certdb ssltestuser ::1/128 cert\n; close HBA; @@ -121,10 +123,6 @@ sub switch_server_cert print SSLCONF ssl_crl_file='root+client.crl'\n; close SSLCONF; - # Stop and restart server to reload the new config. We cannot use - # restart_test_server() because that overrides listen_addresses to only all - # Unix domain socket connections. - - system_or_bail 'pg_ctl', 'stop', '-D', $tempdir/pgdata; - system_or_bail 'pg_ctl', 'start', '-D', $tempdir/pgdata, '-w'; + # Stop and restart server to reload the new config. + restart_test_server(); } diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl index 5d24d8d..0d6f339 100644 --- a/src/test/ssl/t/001_ssltests.pl +++ b/src/test/ssl/t/001_ssltests.pl @@ -27,9 +27,6 @@ my $SERVERHOSTADDR = '127.0.0.1'; my $tempdir = TestLib::tempdir; -#my $tempdir = tmp_check; - - # Define a couple of helper functions to test connecting to the server. my $common_connstr; @@ -43,12 +40,7 @@ sub run_test_psql 'psql', '-A', '-t', '-c', SELECT 'connected with $connstr', '-d', $connstr ]; - open CLIENTLOG, $tempdir/client-log - or die Could not open client-log file; - print CLIENTLOG \n# Running test: $connstr $logstring\n; - close CLIENTLOG; - - my $result = run $cmd, '', $tempdir/client-log, '21'; + my $result = run_log($cmd); return $result; } @@ -84,7 +76,7 @@ chmod 0600, ssl/client.key; diag setting up data directory in \$tempdir\...; start_test_server($tempdir); -configure_test_server_for_ssl($tempdir); +configure_test_server_for_ssl($tempdir, $SERVERHOSTADDR); switch_server_cert($tempdir, 'server-cn-only'); ### Part 1. Run client-side tests. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers