From 8bb14cdd33deecc6977cf5638f73e80f76ab658b Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Fri, 7 Oct 2016 12:20:39 +0300 Subject: Don't share SSL_CTX between libpq connections. There were several issues with the old coding: 1. There was a race condition, if two threads opened a connection at the same time. We used a mutex around SSL_CTX_* calls, but that was not enough, e.g. if one thread SSL_CTX_load_verify_locations() with one path, and another thread set it with a different path, before the first thread got to establish the connection. 2. Opening two different connections, with different sslrootcert settings, seemed to fail outright with "SSL error: block type is not 01". Not sure why. 3. We created the SSL object, before calling SSL_CTX_load_verify_locations and SSL_CTX_use_certificate_chain_file on the SSL context. That was wrong, because the options set on the SSL context are propagated to the SSL object, when the SSL object is created. If they are set after the SSL object has already been created, they won't take effect until the next connection. (This is bug #14329) At least some of these could've been fixed while still using a shared context, but it would've been more complicated and error-prone. To keep things simple, let's just use a separate SSL context for each connection, and accept the overhead. Backpatch to all supported versions. Report, analysis and test case by Kacper Zuk. Discussion: <20160920101051.1355.79453@wrigleys.postgresql.org> --- src/test/ssl/Makefile | 6 +++++- src/test/ssl/README | 4 ++++ src/test/ssl/ServerSetup.pm | 6 ++++-- src/test/ssl/ssl/client+client_ca.crt | 25 +++++++++++++++++++++++++ src/test/ssl/t/001_ssltests.pl | 10 +++++++++- 5 files changed, 47 insertions(+), 4 deletions(-) create mode 100644 src/test/ssl/ssl/client+client_ca.crt (limited to 'src/test') diff --git a/src/test/ssl/Makefile b/src/test/ssl/Makefile index 3d992babff0..2b04d825285 100644 --- a/src/test/ssl/Makefile +++ b/src/test/ssl/Makefile @@ -23,7 +23,8 @@ SSLFILES := $(CERTIFICATES:%=ssl/%.key) $(CERTIFICATES:%=ssl/%.crt) \ ssl/client.crl ssl/server.crl ssl/root.crl \ ssl/both-cas-1.crt ssl/both-cas-2.crt \ ssl/root+server_ca.crt ssl/root+server.crl \ - ssl/root+client_ca.crt ssl/root+client.crl + ssl/root+client_ca.crt ssl/root+client.crl \ + ssl/client+client_ca.crt # This target generates all the key and certificate files. sslfiles: $(SSLFILES) @@ -99,6 +100,9 @@ ssl/root+server_ca.crt: ssl/root_ca.crt ssl/server_ca.crt ssl/root+client_ca.crt: ssl/root_ca.crt ssl/client_ca.crt cat $^ > $@ +ssl/client+client_ca.crt: ssl/client.crt ssl/client_ca.crt + cat $^ > $@ + #### CRLs ssl/client.crl: ssl/client-revoked.crt diff --git a/src/test/ssl/README b/src/test/ssl/README index 52bd68f49fa..50fa14e287e 100644 --- a/src/test/ssl/README +++ b/src/test/ssl/README @@ -65,6 +65,10 @@ root+server_ca root+client_ca Contains root_crt and client_ca.crt. For use as server's "ssl_ca_file". +client+client_ca + Contains client.crt and client_ca.crt in that order. For use as client's + certificate chain. + There are also CRLs for each of the CAs: root.crl, server.crl and client.crl. For convenience, all of these keypairs and certificates are included in the diff --git a/src/test/ssl/ServerSetup.pm b/src/test/ssl/ServerSetup.pm index 4e93184eb03..d312880f8b1 100644 --- a/src/test/ssl/ServerSetup.pm +++ b/src/test/ssl/ServerSetup.pm @@ -75,6 +75,7 @@ sub configure_test_server_for_ssl copy_files("ssl/server-*.key", $pgdata); chmod(0600, glob "$pgdata/server-*.key") or die $!; copy_files("ssl/root+client_ca.crt", $pgdata); + copy_files("ssl/root_ca.crt", $pgdata); copy_files("ssl/root+client.crl", $pgdata); # Only accept SSL connections from localhost. Our tests don't depend on this @@ -101,13 +102,14 @@ sub switch_server_cert { my $node = $_[0]; my $certfile = $_[1]; + my $cafile = $_[2] || "root+client_ca"; my $pgdata = $node->data_dir; - diag "Restarting server with certfile \"$certfile\"..."; + diag "Restarting server with certfile \"$certfile\" and cafile \"$cafile\"..."; open SSLCONF, ">$pgdata/sslconfig.conf"; print SSLCONF "ssl=on\n"; - print SSLCONF "ssl_ca_file='root+client_ca.crt'\n"; + print SSLCONF "ssl_ca_file='$cafile.crt'\n"; print SSLCONF "ssl_cert_file='$certfile.crt'\n"; print SSLCONF "ssl_key_file='$certfile.key'\n"; print SSLCONF "ssl_crl_file='root+client.crl'\n"; diff --git a/src/test/ssl/ssl/client+client_ca.crt b/src/test/ssl/ssl/client+client_ca.crt new file mode 100644 index 00000000000..3caada693de --- /dev/null +++ b/src/test/ssl/ssl/client+client_ca.crt @@ -0,0 +1,25 @@ +-----BEGIN CERTIFICATE----- +MIIBxzCCATACAQEwDQYJKoZIhvcNAQEFBQAwQjFAMD4GA1UEAww3VGVzdCBDQSBm +b3IgUG9zdGdyZVNRTCBTU0wgcmVncmVzc2lvbiB0ZXN0IGNsaWVudCBjZXJ0czAe +Fw0xNjA5MTIxNjMwMDFaFw00NDAxMjkxNjMwMDFaMBYxFDASBgNVBAMMC3NzbHRl +c3R1c2VyMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDN3RFl8VWMEBN1Qas0 +w1CFcXdDEbKVNSPsqWHzHIEPoGJv+eUIBK2lQ/Ce8nRCdelO50RsmlbcXBIrjVl6 +BN0RmEeEVclgCdiamYN53LBdc5KWKpKCKn45lCtlZodWt0hNNx1pAmh85jDKpoO9 +ErbCnSU1wODPqnOzdkLU7jBu5QIDAQABMA0GCSqGSIb3DQEBBQUAA4GBABUz+vnu +dD1Q1N/Ezs5DzJeQDtiJb9PNzBHAUPQoXeLvuITcDdyYWc18Yi4fX7gwyD42q2iu +1I0hmm2bNJfujsGbvGYFLuQ4hC2ucAAj2Gm681GhhaNYtfsfHYm9R8GRZFvp40oj +qXpkDkYsPdyVxUyoxJ+M0Ub5VC/k1pQNtIaq +-----END CERTIFICATE----- +-----BEGIN CERTIFICATE----- +MIICCDCCAXGgAwIBAgIBAjANBgkqhkiG9w0BAQUFADBAMT4wPAYDVQQDDDVUZXN0 +IHJvb3QgQ0EgZm9yIFBvc3RncmVTUUwgU1NMIHJlZ3Jlc3Npb24gdGVzdCBzdWl0 +ZTAeFw0xNjA5MTIxNjMwMDFaFw00NDAxMjkxNjMwMDFaMEIxQDA+BgNVBAMMN1Rl +c3QgQ0EgZm9yIFBvc3RncmVTUUwgU1NMIHJlZ3Jlc3Npb24gdGVzdCBjbGllbnQg +Y2VydHMwgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAMI2MXWSb8TZnCLVNYJ+ +19b4noxRmaR1W2zUxl4aTMfiPt9cK06lNY39EPBfjmb7hjxD76w8fLoV/aZ0gOgd +JXFRZvIg7SyM7QVFma0AJAIZayes+ba1odEmBEi378g0mLrjCLqZtBVHfvJxL/6x +6/flSTAn/+09vtELvvLWBePZAgMBAAGjEDAOMAwGA1UdEwQFMAMBAf8wDQYJKoZI +hvcNAQEFBQADgYEAlGC24V2TsiSlo9RIboBZTZqd0raUpKkmVbkwKyqcmecoFfCI +TCmoyJLYyUL5/e3dtn/cGDcaqxaO3qxnstxVEMSrlCGfZdZJ2oouXZMpDy9CkeOM +ypCCx9pc4EmP3mvu64f21+dNCXlhM36pZ1IokeS5jk2FIHUda+m5jlk5o6I= +-----END CERTIFICATE----- diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl index 80e8ea1fe79..dc8e064b257 100644 --- a/src/test/ssl/t/001_ssltests.pl +++ b/src/test/ssl/t/001_ssltests.pl @@ -2,7 +2,7 @@ use strict; use warnings; use PostgresNode; use TestLib; -use Test::More tests => 38; +use Test::More tests => 40; use ServerSetup; use File::Copy; @@ -239,3 +239,11 @@ test_connect_fails( test_connect_fails( "user=ssltestuser sslcert=ssl/client-revoked.crt sslkey=ssl/client-revoked.key" ); + +# intermediate client_ca.crt is provided by client, and isn't in server's ssl_ca_file +switch_server_cert($node, 'server-cn-only', 'root_ca'); +$common_connstr = +"user=ssltestuser dbname=certdb sslkey=ssl/client.key sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR"; + +test_connect_ok("sslmode=require sslcert=ssl/client+client_ca.crt"); +test_connect_fails("sslmode=require sslcert=ssl/client.crt"); -- cgit v1.2.3