Thread: pg_stat_ssl additions
During discussions of alternative SSL implementations, contrib/sslinfo is usually mentioned as something that something needs to be done about. I've looked into adapting some functionality from sslinfo into the pg_stat_ssl view. These two facilities have a lot of overlap but seem mostly oblivious to each other. The attached patch series - Adds a documentation link from sslinfo to pg_stat_ssl. - Adds tests under src/test/ssl/ for the pg_stat_ssl view. - Changes pg_stat_ssl.clientdn to be null if there is no client certificate (as documented, but not implemented). (bug fix) - Adds new fields to pg_stat_ssl: issuerdn and clientserial. These allow uniquely identifying the client certificate. AFAICT, these are the most interesting pieces of information provided by sslinfo but not in pg_stat_ssl. (I don't like the underscore-free naming of these fields, but it matches the existing "clientdn".) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Hello. At Thu, 18 Oct 2018 00:05:15 +0200, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in <398754d8-6bb5-c5cf-e7b8-22e5f0983caf@2ndquadrant.com> > During discussions of alternative SSL implementations, contrib/sslinfo > is usually mentioned as something that something needs to be done about. > I've looked into adapting some functionality from sslinfo into the > pg_stat_ssl view. These two facilities have a lot of overlap but seem > mostly oblivious to each other. > > The attached patch series > > - Adds a documentation link from sslinfo to pg_stat_ssl. > > - Adds tests under src/test/ssl/ for the pg_stat_ssl view. With this change the feature mapping between the two is as follows. ssl_is_used() : .ssl ssl_version() : .version ssl_cipher() : .cipher (none) : .bits (none) : .compression ssl_client_cert_present() : .clientdn IS NOT NULL ssl_client_serial() : .clientserial ssl_client_dn() : .clientdn ssl_issuer_dn() : .issuerdn ssl_client_dn_field() : (none) ssl_issuer_field() : (none) ssl_extension_info() : (none) # I couldn't create x509 v3 certs for uncertain reasons.. Comparing the two codes in pgstatfuncs.c and sslinfo.c, It seems that ssl_client_serial() can be largely simplified using be_tls_get_peer_serial(). X509_NAME_to_text() can be simplified using X509_NAME_to_cstring(). But this would be another patch. By the way, though it is not by this patch, X509_NAME_to_cstring doesn't handle NID_undef from OBJ_obj2nid() and NULL from OBJ_nid2ln(). The latter case feeds NULL to BIO_printf() . I'm not sure it can actually happen. I don't look through all the similar points but it might be better fix it. > - Changes pg_stat_ssl.clientdn to be null if there is no client > certificate (as documented, but not implemented). (bug fix) This reveals DN, serial and issuer DN of the cert to non-superusres. pg_stat_activity hides at least client_addr. Aren't they needed to be hidden? (This will change the existing behavior.) > - Adds new fields to pg_stat_ssl: issuerdn and clientserial. These > allow uniquely identifying the client certificate. AFAICT, these are > the most interesting pieces of information provided by sslinfo but not > in pg_stat_ssl. (I don't like the underscore-free naming of these > fields, but it matches the existing "clientdn".) clientdn, clientserial, issuerdn are the fields about client certificates. Only the last one omits prefixing "client". But "clientissuerdn" seems somewhat rotten... Counldn't we rename clientdn to client_dn? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On 20/11/2018 09:31, Kyotaro HORIGUCHI wrote: > Comparing the two codes in pgstatfuncs.c and sslinfo.c, It seems > that ssl_client_serial() can be largely simplified using > be_tls_get_peer_serial(). X509_NAME_to_text() can be simplified > using X509_NAME_to_cstring(). But this would be another patch. > > By the way, though it is not by this patch, X509_NAME_to_cstring > doesn't handle NID_undef from OBJ_obj2nid() and NULL from > OBJ_nid2ln(). The latter case feeds NULL to BIO_printf() . I'm > not sure it can actually happen. Yes, that seems problematic, at least in theory. >> - Changes pg_stat_ssl.clientdn to be null if there is no client >> certificate (as documented, but not implemented). (bug fix) > > This reveals DN, serial and issuer DN of the cert to > non-superusres. pg_stat_activity hides at least > client_addr. Aren't they needed to be hidden? (This will change > the existing behavior.) Yes. Arguably, nothing in this view other than your own session should be seen by normal users. Thoughts from others? >> - Adds new fields to pg_stat_ssl: issuerdn and clientserial. These >> allow uniquely identifying the client certificate. AFAICT, these are >> the most interesting pieces of information provided by sslinfo but not >> in pg_stat_ssl. (I don't like the underscore-free naming of these >> fields, but it matches the existing "clientdn".) > > clientdn, clientserial, issuerdn are the fields about client > certificates. Only the last one omits prefixing "client". But > "clientissuerdn" seems somewhat rotten... Counldn't we rename > clientdn to client_dn? I'd prefer renaming as well, but some people might not like that. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Oct 18, 2018 at 11:05 AM Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > - Adds tests under src/test/ssl/ for the pg_stat_ssl view. Hi Peter, Your new tests fail when run by cfbot (Ubuntu), and also on my Debian buster machine, but pass on my FreeBSD 13-CURRENT box. I think the problem is that you match cipher names with \w+, but some cipher names sometimes (on some library versions?) have hyphens in them instead of underscores. https://travis-ci.org/postgresql-cfbot/postgresql/builds/459620336 -- Thomas Munro http://www.enterprisedb.com
On 27/11/2018 00:29, Thomas Munro wrote: > On Thu, Oct 18, 2018 at 11:05 AM Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: >> - Adds tests under src/test/ssl/ for the pg_stat_ssl view. > > Hi Peter, > > Your new tests fail when run by cfbot (Ubuntu), and also on my Debian > buster machine, but pass on my FreeBSD 13-CURRENT box. I think the > problem is that you match cipher names with \w+, but some cipher names > sometimes (on some library versions?) have hyphens in them instead of > underscores. Right, this is a TLSv1.3 vs earlier thing. Updated patches attached. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On 20/11/2018 22:41, Peter Eisentraut wrote: >>> - Adds new fields to pg_stat_ssl: issuerdn and clientserial. These >>> allow uniquely identifying the client certificate. AFAICT, these are >>> the most interesting pieces of information provided by sslinfo but not >>> in pg_stat_ssl. (I don't like the underscore-free naming of these >>> fields, but it matches the existing "clientdn".) >> clientdn, clientserial, issuerdn are the fields about client >> certificates. Only the last one omits prefixing "client". But >> "clientissuerdn" seems somewhat rotten... Counldn't we rename >> clientdn to client_dn? > I'd prefer renaming as well, but some people might not like that. Any thoughts from others about whether to rename clientdn to client_dn to allow better naming of the new fields? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Nov 28, 2018 at 06:31:59PM +0100, Peter Eisentraut wrote: > On 20/11/2018 22:41, Peter Eisentraut wrote: > >>> - Adds new fields to pg_stat_ssl: issuerdn and clientserial. These > >>> allow uniquely identifying the client certificate. AFAICT, these are > >>> the most interesting pieces of information provided by sslinfo but not > >>> in pg_stat_ssl. (I don't like the underscore-free naming of these > >>> fields, but it matches the existing "clientdn".) > >> clientdn, clientserial, issuerdn are the fields about client > >> certificates. Only the last one omits prefixing "client". But > >> "clientissuerdn" seems somewhat rotten... Counldn't we rename > >> clientdn to client_dn? > > I'd prefer renaming as well, but some people might not like that. > > Any thoughts from others about whether to rename clientdn to client_dn > to allow better naming of the new fields? Makes sense. The SSL acronyms can get very complex. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Greetings, * Bruce Momjian (bruce@momjian.us) wrote: > On Wed, Nov 28, 2018 at 06:31:59PM +0100, Peter Eisentraut wrote: > > On 20/11/2018 22:41, Peter Eisentraut wrote: > > >>> - Adds new fields to pg_stat_ssl: issuerdn and clientserial. These > > >>> allow uniquely identifying the client certificate. AFAICT, these are > > >>> the most interesting pieces of information provided by sslinfo but not > > >>> in pg_stat_ssl. (I don't like the underscore-free naming of these > > >>> fields, but it matches the existing "clientdn".) > > >> clientdn, clientserial, issuerdn are the fields about client > > >> certificates. Only the last one omits prefixing "client". But > > >> "clientissuerdn" seems somewhat rotten... Counldn't we rename > > >> clientdn to client_dn? > > > I'd prefer renaming as well, but some people might not like that. > > > > Any thoughts from others about whether to rename clientdn to client_dn > > to allow better naming of the new fields? > > Makes sense. The SSL acronyms can get very complex. Agreed. Thanks! Stephen
Attachment
Bruce Momjian <bruce@momjian.us> writes:
> On Wed, Nov 28, 2018 at 06:31:59PM +0100, Peter Eisentraut wrote:
>> Any thoughts from others about whether to rename clientdn to client_dn
>> to allow better naming of the new fields?
> Makes sense. The SSL acronyms can get very complex.
+1. It seems unlikely to me that there are very many applications out
there that have references to this view, so we can probably get away
with rationalizing the field names.
regards, tom lane
As we do make significant(?) use of the ssl-ish stuff - though not of the views - should I weigh in?
We do make some not-insignificant use of the sslinfo data, but I see little issue with adding underscores. In fact, ssl-ville is replete with underscores anyway.
Further, I’m not sure exposing details about Cert Issuer, etc. to non-privileged users is much of an issue. For the most part, in most use cases, ‘users’ should/would want to know what entity is the issuer. If we’re talking about client certs, most of this is readily readable anyway, no?
More from PostgreSQL == better.
Lou Picciano
PS - How you guys doin’? It’s been a while.
On Nov 28, 2018, at 4:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:Bruce Momjian <bruce@momjian.us> writes:On Wed, Nov 28, 2018 at 06:31:59PM +0100, Peter Eisentraut wrote:Any thoughts from others about whether to rename clientdn to client_dn
to allow better naming of the new fields?Makes sense. The SSL acronyms can get very complex.
+1. It seems unlikely to me that there are very many applications out
there that have references to this view, so we can probably get away
with rationalizing the field names.
regards, tom lane
On 29/11/2018 01:27, Lou Picciano wrote: > Further, I’m not sure exposing details about Cert Issuer, etc. to > non-privileged users is much of an issue. For the most part, in most use > cases, ‘users’ should//would/ want to know what entity is the issuer. If > we’re talking about client certs, most of this is readily readable > anyway, no? The debate is whether an unprivileged user should be able to read the SSL information of *other* users' connections. My opinion is no. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Updated patch with the renamed columns. On 29/11/2018 14:49, Peter Eisentraut wrote: > On 29/11/2018 01:27, Lou Picciano wrote: >> Further, I’m not sure exposing details about Cert Issuer, etc. to >> non-privileged users is much of an issue. For the most part, in most use >> cases, ‘users’ should//would/ want to know what entity is the issuer. If >> we’re talking about client certs, most of this is readily readable >> anyway, no? > > The debate is whether an unprivileged user should be able to read the > SSL information of *other* users' connections. > > My opinion is no. I propose to address this as a separate patch later on. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Updated patches for some merge conflicts -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Hello, thank you for the new version. At Fri, 4 Jan 2019 00:25:57 +0100, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in <3f8a7a73-cebe-016f-49e3-853733f8baf2@2ndquadrant.com> > Updated patches for some merge conflicts 0001: Seems perfect to me. 0002: The test 54-56 of 001_ssltest.pl failed, which succeeded before applying 0002. Seems to need to use another user. # Failed test 'pg_stat_ssl view without client certificate: no stderr' # at t/001_ssltests.pl line 313. # got: 'psql: SSL error: certificate verify failed # FATAL: no pg_hba.conf entry for host "127.0.0.1", user "ssltestuser", database "trustdb", SSL off # ' If this is not specific to my environment, the connevcion string at line 313 of 001_ssltests.pl needs sslrootcert setting (, which is feeded to test_connect_ok/fails() via $connstr, not via $common_connstr). 0003: Looks fine to me. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Sorry, I left a garbage. At Mon, 28 Jan 2019 17:14:00 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190128.171400.111796002.horiguchi.kyotaro@lab.ntt.co.jp> > Hello, thank you for the new version. > > At Fri, 4 Jan 2019 00:25:57 +0100, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in <3f8a7a73-cebe-016f-49e3-853733f8baf2@2ndquadrant.com> > > Updated patches for some merge conflicts > > 0001: Seems perfect to me. > > 0002: > > The test 54-56 of 001_ssltest.pl failed, which succeeded before > applying 0002. Seems to need to use another user. "Seems to need to use another user." is useles leftover. > # Failed test 'pg_stat_ssl view without client certificate: no stderr' > # at t/001_ssltests.pl line 313. > # got: 'psql: SSL error: certificate verify failed > # FATAL: no pg_hba.conf entry for host "127.0.0.1", user "ssltestuser", database "trustdb", SSL off > # ' > > If this is not specific to my environment, the connevcion string > at line 313 of 001_ssltests.pl needs sslrootcert setting (, which > is feeded to test_connect_ok/fails() via $connstr, not via > $common_connstr). > > 0003: Looks fine to me. > > regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On 28/01/2019 09:14, Kyotaro HORIGUCHI wrote: > 0002: > > The test 54-56 of 001_ssltest.pl failed, which succeeded before > applying 0002. Seems to need to use another user. > > # Failed test 'pg_stat_ssl view without client certificate: no stderr' > # at t/001_ssltests.pl line 313. > # got: 'psql: SSL error: certificate verify failed > # FATAL: no pg_hba.conf entry for host "127.0.0.1", user "ssltestuser", database "trustdb", SSL off > # ' > > If this is not specific to my environment, the connevcion string > at line 313 of 001_ssltests.pl needs sslrootcert setting (, which > is feeded to test_connect_ok/fails() via $connstr, not via > $common_connstr). This is strange. The tests work for me, and also on the cfbot. The pg_hba.conf method is "trust", and there is nothing that should make it do certificate verification for this test. Do you have have any PGSSL* environment variables set perhaps? An interesting OpenSSL version or configuration perhaps? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
At Mon, 28 Jan 2019 14:53:43 +0100, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in
<24783370-5acd-e0f3-8eb7-7f42ff2a026d@2ndquadrant.com>
> On 28/01/2019 09:14, Kyotaro HORIGUCHI wrote:
> > 0002:
> >
> > The test 54-56 of 001_ssltest.pl failed, which succeeded before
> > applying 0002. Seems to need to use another user.
> >
> > # Failed test 'pg_stat_ssl view without client certificate: no stderr'
> > # at t/001_ssltests.pl line 313.
> > # got: 'psql: SSL error: certificate verify failed
> > # FATAL: no pg_hba.conf entry for host "127.0.0.1", user "ssltestuser", database "trustdb", SSL off
> > # '
> >
> > If this is not specific to my environment, the connevcion string
> > at line 313 of 001_ssltests.pl needs sslrootcert setting (, which
> > is feeded to test_connect_ok/fails() via $connstr, not via
> > $common_connstr).
>
> This is strange. The tests work for me, and also on the cfbot. The
Agreed. It seemed so also to me.
> pg_hba.conf method is "trust", and there is nothing that should make it
> do certificate verification for this test. Do you have have any PGSSL*
> environment variables set perhaps? An interesting OpenSSL version or
> configuration perhaps?
Some further investigation told me that the file
~/.postgresql/root.cert was the culprit.
When initializing SSL context, it picks up the root certificate
from my home directory, not in test installation and I had one
there. It is not based on $HOME but pwent so it is unchangeable
(and it is the right design for the purpose).
sslcert, sslkey, sslrootcert and sslcrl are in the same
characteristic so they should be set to invalid value (namely
"invalid") if not used.
The attached diff file on top of 0002 adds a new variable
$def_connstr for the properties above and some other variables,
then uses it as the first part of $common_connstr.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 2bcbb1b42a..aa0692cc47 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -25,6 +25,13 @@ my $SERVERHOSTADDR = '127.0.0.1';
# Allocation of base connection string shared among multiple tests.
my $common_connstr;
+# ssl-related properties may defautly set to the files in the users'
+# environment. Explicitly provide them a value so that they don't
+# point a valid file accidentially. Some other common properties are
+# set here together.
+# Attach this at the head of $common_connstr.
+my $def_connstr = "user=ssltestuser dbname=trustdb sslcert=invalid sslkey=invalid sslrootcert=invalid sslcrl=invalid
";
+
# The client's private key must not be world-readable, so take a copy
# of the key stored in the code tree and update its permissions.
copy("ssl/client.key", "ssl/client_tmp.key");
@@ -93,7 +100,7 @@ note "running client tests";
switch_server_cert($node, 'server-cn-only');
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
+ $def_connstr."hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
# The server should not accept non-SSL connections.
test_connect_fails(
@@ -185,7 +192,7 @@ test_connect_ok(
# Check that connecting with verify-full fails, when the hostname doesn't
# match the hostname in the server's certificate.
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
+ $def_connstr."sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
test_connect_ok(
$common_connstr,
@@ -205,7 +212,7 @@ test_connect_fails(
switch_server_cert($node, 'server-multiple-alt-names');
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR
sslmode=verify-full";
+ $def_connstr."sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full";
test_connect_ok(
$common_connstr,
@@ -236,7 +243,7 @@ test_connect_fails(
switch_server_cert($node, 'server-single-alt-name');
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR
sslmode=verify-full";
+ $def_connstr."sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full";
test_connect_ok(
$common_connstr,
@@ -260,7 +267,7 @@ test_connect_fails(
switch_server_cert($node, 'server-cn-and-alt-names');
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR
sslmode=verify-full";
+ $def_connstr."sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full";
test_connect_ok(
$common_connstr,
@@ -280,7 +287,7 @@ test_connect_fails(
# not a very sensible certificate, but libpq should handle it gracefully.
switch_server_cert($node, 'server-no-names');
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
+ $def_connstr."sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
test_connect_ok(
$common_connstr,
@@ -296,7 +303,7 @@ test_connect_fails(
switch_server_cert($node, 'server-revoked');
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
+ $def_connstr."hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
# Without the CRL, succeeds. With it, fails.
test_connect_ok(
@@ -326,7 +333,7 @@ command_like([
note "running server tests";
$common_connstr =
- "sslrootcert=ssl/root+server_ca.crt sslmode=require dbname=certdb hostaddr=$SERVERHOSTADDR";
+ $def_connstr."sslrootcert=ssl/root+server_ca.crt sslmode=require dbname=certdb hostaddr=$SERVERHOSTADDR";
# no client cert
test_connect_fails(
@@ -376,7 +383,7 @@ test_connect_fails(
# 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_tmp.key sslrootcert=ssl/root+server_ca.crt
hostaddr=$SERVERHOSTADDR";
+ $def_connstr."dbname=certdb sslkey=ssl/client_tmp.key sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
test_connect_ok(
$common_connstr,
On Tue, Jan 29, 2019 at 12:18:29PM +0900, Kyotaro HORIGUCHI wrote: > At Mon, 28 Jan 2019 14:53:43 +0100, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in <24783370-5acd-e0f3-8eb7-7f42ff2a026d@2ndquadrant.com> >> This is strange. The tests work for me, and also on the cfbot. The > > Agreed. It seemed so also to me. The tests look sane to me for what it's worth. > When initializing SSL context, it picks up the root certificate > from my home directory, not in test installation and I had one > there. It is not based on $HOME but pwent so it is unchangeable > (and it is the right design for the purpose). Oops. I agree that the tests ought to be as much isolated as possible, without optionally fetching things depending on the user environment. > +# ssl-related properties may defautly set to the files in the users' > +# environment. Explicitly provide them a value so that they don't > +# point a valid file accidentially. Some other common properties are > +# set here together. > +# Attach this at the head of $common_connstr. > +my $def_connstr = "user=ssltestuser dbname=trustdb sslcert=invalid sslkey=invalid sslrootcert=invalid sslcrl=invalid "; > + Maybe it is better to just put that in test_connect_ok and test_connect_fails for only the SSL parameters? I don't see much point in enforcing dbname and user. > # The server should not accept non-SSL connections. > test_connect_fails( > @@ -185,7 +192,7 @@ test_connect_ok( > # Check that connecting with verify-full fails, when the hostname doesn't > # match the hostname in the server's certificate. > $common_connstr = > - "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR"; > + $def_connstr."sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR"; I think this is bad, inconsistent style. Adding the variable within the quoted section is fine, as much as moving SERVERHOSTADDR out. But mixing styles is not. -- Michael
Attachment
On 29/01/2019 04:18, Kyotaro HORIGUCHI wrote: > Some further investigation told me that the file > ~/.postgresql/root.cert was the culprit. OK, I could reproduce the problem and found a fix for it. Basically you need to specify sslrootcert in each test, and these new tests didn't do it. All other tests were OK, so a local fix was enough. I have committed 0001..0003 now. Attached is the latest version of 0004, about which I didn't not get an explicit comment in your last review message. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
At Tue, 29 Jan 2019 13:50:17 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20190129045017.GC3121@paquier.xyz>
> On Tue, Jan 29, 2019 at 12:18:29PM +0900, Kyotaro HORIGUCHI wrote:
> > At Mon, 28 Jan 2019 14:53:43 +0100, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in
<24783370-5acd-e0f3-8eb7-7f42ff2a026d@2ndquadrant.com>
> >> This is strange. The tests work for me, and also on the cfbot. The
> >
> > Agreed. It seemed so also to me.
>
> The tests look sane to me for what it's worth.
>
> > When initializing SSL context, it picks up the root certificate
> > from my home directory, not in test installation and I had one
> > there. It is not based on $HOME but pwent so it is unchangeable
> > (and it is the right design for the purpose).
>
> Oops. I agree that the tests ought to be as much isolated as
> possible, without optionally fetching things depending on the user
> environment.
>
> > +# ssl-related properties may defautly set to the files in the users'
> > +# environment. Explicitly provide them a value so that they don't
> > +# point a valid file accidentially. Some other common properties are
> > +# set here together.
> > +# Attach this at the head of $common_connstr.
> > +my $def_connstr = "user=ssltestuser dbname=trustdb sslcert=invalid sslkey=invalid sslrootcert=invalid
sslcrl=invalid";
> > +
>
> Maybe it is better to just put that in test_connect_ok and
> test_connect_fails for only the SSL parameters? I don't see much
> point in enforcing dbname and user.
Hmm. It is the first thing I did for the issue. Anyway..
Of course dbname and user are not necessary to fix it. Ok, I
attached two patches. The first applies on the current master and
prevent psql from loading ssl-related files from out of testing
environment. The second applies on the 0002 patch and prevent the
patch from loading a wrong sslrootcert.
> > # The server should not accept non-SSL connections.
> > test_connect_fails(
> > @@ -185,7 +192,7 @@ test_connect_ok(
> > # Check that connecting with verify-full fails, when the hostname doesn't
> > # match the hostname in the server's certificate.
> > $common_connstr =
> > - "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
> > + $def_connstr."sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
>
> I think this is bad, inconsistent style. Adding the variable within
> the quoted section is fine, as much as moving SERVERHOSTADDR out. But
> mixing styles is not.
I Understood. Thanks for the suggestion.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
From a3cb52ba165074db1d2910faa8e58030d84fa7c9 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Tue, 29 Jan 2019 21:06:58 +0900
Subject: [PATCH 1/4] Prevent from loading ssl files out of testing
environment.
SSL test script can let psql load SSL-related files out of the testing
environemnt. Fix it by explicitly setting sslcert, sslrootcert and
sslcrl when they are not used. sslkey doesn't need the fix because it
is loaded only when sslcert is loadded.
---
src/test/ssl/t/001_ssltests.pl | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 2b875a3c95..114541590d 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -93,7 +93,7 @@ note "running client tests";
switch_server_cert($node, 'server-cn-only');
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
+ "user=ssltestuser dbname=trustdb sslcert=invalid sslcrl=invalid hostaddr=$SERVERHOSTADDR
host=common-name.pg-ssltest.test";
# The server should not accept non-SSL connections.
test_connect_fails(
@@ -185,7 +185,7 @@ test_connect_ok(
# Check that connecting with verify-full fails, when the hostname doesn't
# match the hostname in the server's certificate.
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
+ "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt sslcrl=invalid
hostaddr=$SERVERHOSTADDR";
test_connect_ok(
$common_connstr,
@@ -205,7 +205,7 @@ test_connect_fails(
switch_server_cert($node, 'server-multiple-alt-names');
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR
sslmode=verify-full";
+ "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt sslcrl=invalid
hostaddr=$SERVERHOSTADDRsslmode=verify-full";
test_connect_ok(
$common_connstr,
@@ -236,7 +236,7 @@ test_connect_fails(
switch_server_cert($node, 'server-single-alt-name');
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR
sslmode=verify-full";
+ "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt sslcrl=invalid
hostaddr=$SERVERHOSTADDRsslmode=verify-full";
test_connect_ok(
$common_connstr,
@@ -260,7 +260,7 @@ test_connect_fails(
switch_server_cert($node, 'server-cn-and-alt-names');
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR
sslmode=verify-full";
+ "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt sslcrl=invalid
hostaddr=$SERVERHOSTADDRsslmode=verify-full";
test_connect_ok(
$common_connstr,
@@ -280,7 +280,7 @@ test_connect_fails(
# not a very sensible certificate, but libpq should handle it gracefully.
switch_server_cert($node, 'server-no-names');
$common_connstr =
- "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
+ "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt sslcrl=invalid
hostaddr=$SERVERHOSTADDR";
test_connect_ok(
$common_connstr,
@@ -301,7 +301,7 @@ $common_connstr =
# Without the CRL, succeeds. With it, fails.
test_connect_ok(
$common_connstr,
- "sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca",
+ "sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrl=invalid",
"connects without client-side CRL");
test_connect_fails(
$common_connstr,
@@ -316,7 +316,7 @@ test_connect_fails(
note "running server tests";
$common_connstr =
- "sslrootcert=ssl/root+server_ca.crt sslmode=require dbname=certdb hostaddr=$SERVERHOSTADDR";
+ "sslrootcert=ssl/root+server_ca.crt sslcrl=invalid sslmode=require dbname=certdb hostaddr=$SERVERHOSTADDR";
# no client cert
test_connect_fails(
@@ -356,7 +356,7 @@ test_connect_fails(
# 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_tmp.key sslrootcert=ssl/root+server_ca.crt
hostaddr=$SERVERHOSTADDR";
+ "user=ssltestuser dbname=certdb sslkey=ssl/client_tmp.key sslrootcert=ssl/root+server_ca.crt sslcrl=invalid
hostaddr=$SERVERHOSTADDR";
test_connect_ok(
$common_connstr,
--
2.16.3
From 47ab02f905fa43479c8e44e89b857b311e175363 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Tue, 29 Jan 2019 21:00:29 +0900
Subject: [PATCH 4/4] Don't load wrong sslrootcert
Fixes the pg_stat_ssl patch so that 0002 don't load sslrootcert out of
testing environment.
---
src/test/ssl/t/001_ssltests.pl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index dea25073b5..7bf9834964 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -312,7 +312,7 @@ test_connect_fails(
# pg_stat_ssl
command_like([
'psql', '-X', '-A', '-F', ',', '-P', 'null=_null_',
- '-d', $common_connstr,
+ '-d', "$common_connstr sslrootcert=invalid",
'-c', "SELECT * FROM pg_stat_ssl WHERE pid = pg_backend_pid()"
],
qr{^pid,ssl,version,cipher,bits,compression,clientdn\n
--
2.16.3
On 29/01/2019 13:11, Peter Eisentraut wrote: > On 29/01/2019 04:18, Kyotaro HORIGUCHI wrote: >> Some further investigation told me that the file >> ~/.postgresql/root.cert was the culprit. > > OK, I could reproduce the problem and found a fix for it. Basically you > need to specify sslrootcert in each test, and these new tests didn't do > it. All other tests were OK, so a local fix was enough. > > I have committed 0001..0003 now. Attached is the latest version of > 0004, about which I didn't not get an explicit comment in your last > review message. I have committed the rest of this. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services