pg_stat_ssl additions

Lists: pgsql-hackers
From: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Subject: pg_stat_ssl additions
Date: 2018-10-17 22:05:15
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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 Content-Type Size
v1-0001-doc-Add-link-from-sslinfo-to-pg_stat_ssl.patch text/plain 941 bytes
v1-0002-Add-tests-for-pg_stat_ssl-system-view.patch text/plain 1.7 KB
v1-0003-Fix-pg_stat_ssl.clientdn.patch text/plain 1.7 KB
v1-0004-Add-more-columns-to-pg_stat_ssl.patch text/plain 13.9 KB

From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: peter(dot)eisentraut(at)2ndquadrant(dot)com
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_stat_ssl additions
Date: 2018-11-20 08:31:22
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hello.

At Thu, 18 Oct 2018 00:05:15 +0200, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote in <398754d8-6bb5-c5cf-e7b8-22e5f0983caf(at)2ndquadrant(dot)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


From: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
To: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_stat_ssl additions
Date: 2018-11-20 21:41:51
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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


From: Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
To: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
Cc: Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_stat_ssl additions
Date: 2018-11-26 23:29:44
Message-ID: CAEepm=1syKaBTncZfLzr=cbSww2CM6TvDWMS4UkrnVou-CF54Q@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Thu, Oct 18, 2018 at 11:05 AM Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)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


From: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
To: Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
Cc: Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_stat_ssl additions
Date: 2018-11-28 17:30:38
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 27/11/2018 00:29, Thomas Munro wrote:
> On Thu, Oct 18, 2018 at 11:05 AM Peter Eisentraut
> <peter(dot)eisentraut(at)2ndquadrant(dot)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 Content-Type Size
v2-0001-doc-Add-link-from-sslinfo-to-pg_stat_ssl.patch text/plain 941 bytes
v2-0002-Add-tests-for-pg_stat_ssl-system-view.patch text/plain 1.7 KB
v2-0003-Fix-pg_stat_ssl.clientdn.patch text/plain 1.7 KB
v2-0004-Add-more-columns-to-pg_stat_ssl.patch text/plain 13.9 KB

From: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
To: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_stat_ssl additions
Date: 2018-11-28 17:31:59
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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


From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
Cc: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_stat_ssl additions
Date: 2018-11-28 17:34:54
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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(at)momjian(dot)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 +


From: Stephen Frost <sfrost(at)snowman(dot)net>
To: Bruce Momjian <bruce(at)momjian(dot)us>
Cc: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_stat_ssl additions
Date: 2018-11-28 17:39:28
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Greetings,

* Bruce Momjian (bruce(at)momjian(dot)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


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Bruce Momjian <bruce(at)momjian(dot)us>
Cc: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_stat_ssl additions
Date: 2018-11-28 21:01:31
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Bruce Momjian <bruce(at)momjian(dot)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


From: Lou Picciano <LouPicciano(at)comcast(dot)net>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Bruce Momjian <bruce(at)momjian(dot)us>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_stat_ssl additions
Date: 2018-11-29 00:27:00
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Bruce Momjian <bruce(at)momjian(dot)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
>


From: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
To: Lou Picciano <LouPicciano(at)comcast(dot)net>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Bruce Momjian <bruce(at)momjian(dot)us>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_stat_ssl additions
Date: 2018-11-29 13:49:02
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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


From: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
To: Lou Picciano <LouPicciano(at)comcast(dot)net>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Bruce Momjian <bruce(at)momjian(dot)us>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_stat_ssl additions
Date: 2018-12-01 13:41:42
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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 Content-Type Size
v3-0001-doc-Add-link-from-sslinfo-to-pg_stat_ssl.patch text/plain 941 bytes
v3-0002-Add-tests-for-pg_stat_ssl-system-view.patch text/plain 1.7 KB
v3-0003-Fix-pg_stat_ssl.clientdn.patch text/plain 1.7 KB
v3-0004-Add-more-columns-to-pg_stat_ssl.patch text/plain 14.6 KB

From: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
To: Lou Picciano <LouPicciano(at)comcast(dot)net>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Bruce Momjian <bruce(at)momjian(dot)us>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_stat_ssl additions
Date: 2019-01-03 23:25:57
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Updated patches for some merge conflicts

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachment Content-Type Size
v4-0001-doc-Add-link-from-sslinfo-to-pg_stat_ssl.patch text/plain 941 bytes
v4-0002-Add-tests-for-pg_stat_ssl-system-view.patch text/plain 1.7 KB
v4-0003-Fix-pg_stat_ssl.clientdn.patch text/plain 1.7 KB
v4-0004-Add-more-columns-to-pg_stat_ssl.patch text/plain 14.7 KB

From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: peter(dot)eisentraut(at)2ndquadrant(dot)com
Cc: LouPicciano(at)comcast(dot)net, tgl(at)sss(dot)pgh(dot)pa(dot)us, bruce(at)momjian(dot)us, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_stat_ssl additions
Date: 2019-01-28 08:14:00
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hello, thank you for the new version.

At Fri, 4 Jan 2019 00:25:57 +0100, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote in <3f8a7a73-cebe-016f-49e3-853733f8baf2(at)2ndquadrant(dot)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


From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: peter(dot)eisentraut(at)2ndquadrant(dot)com
Cc: LouPicciano(at)comcast(dot)net, tgl(at)sss(dot)pgh(dot)pa(dot)us, bruce(at)momjian(dot)us, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_stat_ssl additions
Date: 2019-01-28 08:19:02
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Sorry, I left a garbage.

At Mon, 28 Jan 2019 17:14:00 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20190128(dot)171400(dot)111796002(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> Hello, thank you for the new version.
>
> At Fri, 4 Jan 2019 00:25:57 +0100, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote in <3f8a7a73-cebe-016f-49e3-853733f8baf2(at)2ndquadrant(dot)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


From: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
To: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: LouPicciano(at)comcast(dot)net, tgl(at)sss(dot)pgh(dot)pa(dot)us, bruce(at)momjian(dot)us, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_stat_ssl additions
Date: 2019-01-28 13:53:43
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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


From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: peter(dot)eisentraut(at)2ndquadrant(dot)com
Cc: LouPicciano(at)comcast(dot)net, tgl(at)sss(dot)pgh(dot)pa(dot)us, bruce(at)momjian(dot)us, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_stat_ssl additions
Date: 2019-01-29 03:18:29
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

At Mon, 28 Jan 2019 14:53:43 +0100, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote in <24783370-5acd-e0f3-8eb7-7f42ff2a026d(at)2ndquadrant(dot)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

Attachment Content-Type Size
dont_use_default_cert_files.patch text/x-patch 4.4 KB

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: peter(dot)eisentraut(at)2ndquadrant(dot)com, LouPicciano(at)comcast(dot)net, tgl(at)sss(dot)pgh(dot)pa(dot)us, bruce(at)momjian(dot)us, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_stat_ssl additions
Date: 2019-01-29 04:50:17
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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(dot)eisentraut(at)2ndquadrant(dot)com> wrote in <24783370-5acd-e0f3-8eb7-7f42ff2a026d(at)2ndquadrant(dot)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


From: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
To: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: LouPicciano(at)comcast(dot)net, tgl(at)sss(dot)pgh(dot)pa(dot)us, bruce(at)momjian(dot)us, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_stat_ssl additions
Date: 2019-01-29 12:11:44
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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 Content-Type Size
v5-0004-Add-more-columns-to-pg_stat_ssl.patch text/plain 14.8 KB

From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: michael(at)paquier(dot)xyz
Cc: peter(dot)eisentraut(at)2ndquadrant(dot)com, LouPicciano(at)comcast(dot)net, tgl(at)sss(dot)pgh(dot)pa(dot)us, bruce(at)momjian(dot)us, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_stat_ssl additions
Date: 2019-01-29 12:21:01
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

At Tue, 29 Jan 2019 13:50:17 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20190129045017(dot)GC3121(at)paquier(dot)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(dot)eisentraut(at)2ndquadrant(dot)com> wrote in <24783370-5acd-e0f3-8eb7-7f42ff2a026d(at)2ndquadrant(dot)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

Attachment Content-Type Size
0001-Prevent-from-loading-ssl-files-out-of-testing-enviro.patch text/x-patch 4.5 KB
0002-Don-t-load-wrong-sslrootcert.patch text/x-patch 930 bytes

From: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
To: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: LouPicciano(at)comcast(dot)net, tgl(at)sss(dot)pgh(dot)pa(dot)us, bruce(at)momjian(dot)us, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_stat_ssl additions
Date: 2019-01-31 23:48:19
Message-ID: [email protected]
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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