primary_conninfo missing from pg_stat_wal_receiver

On Sun, Jun 19, 2016 at 5:56 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

The new pg_stat_wal_receiver does not include primary_conninfo.
Looking at that now, it looks almost stupid not to include it...
Adding it now would require a catalog bump, so I am not sure if this
is acceptable at this stage for 9.6...

Or in short the attached.
--
Michael

On 19/06/16 12:28, Michael Paquier wrote:

On Sun, Jun 19, 2016 at 5:56 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

The new pg_stat_wal_receiver does not include primary_conninfo.
Looking at that now, it looks almost stupid not to include it...
Adding it now would require a catalog bump, so I am not sure if this
is acceptable at this stage for 9.6...

This definitely needs to go in, we get regular requests for it. The
last one I've seen being
/messages/by-id/CAN1xZseiqNRh1ZE0seVk7UuHeWvFBEWG+FcW7Xfm_Nv=d+fPGw@mail.gmail.com

Whether it goes into 9.6 or 10 is not for me to say.

Or in short the attached.

The code looks good to me but why no documentation?
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, Jun 19, 2016 at 7:38 PM, Vik Fearing <vik@2ndquadrant.fr> wrote:

On 19/06/16 12:28, Michael Paquier wrote:

On Sun, Jun 19, 2016 at 5:56 PM, Michael Paquier
Or in short the attached.

The code looks good to me but why no documentation?

Because that's a long day... Thanks! Now you have it.
--
Michael

On 19/06/16 13:02, Michael Paquier wrote:

On Sun, Jun 19, 2016 at 7:38 PM, Vik Fearing <vik@2ndquadrant.fr> wrote:

On 19/06/16 12:28, Michael Paquier wrote:

On Sun, Jun 19, 2016 at 5:56 PM, Michael Paquier
Or in short the attached.

The code looks good to me but why no documentation?

Because that's a long day... Thanks! Now you have it.

Thanks, this looks good. Could you please add it to the next commitfest
so that it doesn't get lost and also so I can do an official review of it?
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 19/06/16 13:02, Michael Paquier wrote:

On Sun, Jun 19, 2016 at 7:38 PM, Vik Fearing <vik@2ndquadrant.fr> wrote:

On 19/06/16 12:28, Michael Paquier wrote:

On Sun, Jun 19, 2016 at 5:56 PM, Michael Paquier
Or in short the attached.

The code looks good to me but why no documentation?

Because that's a long day... Thanks! Now you have it.

Quick bikeshed: I think the column should be called conninfo and not
conn_info to match other places it's used.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Jun 20, 2016 at 11:26 PM, Vik Fearing <vik@2ndquadrant.fr> wrote:

On 19/06/16 13:02, Michael Paquier wrote:

On Sun, Jun 19, 2016 at 7:38 PM, Vik Fearing <vik@2ndquadrant.fr> wrote:

On 19/06/16 12:28, Michael Paquier wrote:

On Sun, Jun 19, 2016 at 5:56 PM, Michael Paquier
Or in short the attached.

The code looks good to me but why no documentation?

Because that's a long day... Thanks! Now you have it.

Thanks, this looks good. Could you please add it to the next commitfest
so that it doesn't get lost and also so I can do an official review of it?

Yes, I just did that. That's too late for 9.6 anyway with beta2 close by.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Thanks, this looks good. Could you please add it to the next commitfest
so that it doesn't get lost and also so I can do an official review of it?

Yes, I just did that. That's too late for 9.6 anyway with beta2 close by.

Even there seems to be ongoing discussions on changing version number
while in the beta period (and which definitely requires initdb). Why
not changing system catalog during beta?:-)

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Jun 21, 2016 at 9:58 AM, Tatsuo Ishii <ishii@postgresql.org> wrote:

Thanks, this looks good. Could you please add it to the next commitfest
so that it doesn't get lost and also so I can do an official review of it?

Yes, I just did that. That's too late for 9.6 anyway with beta2 close by.

Even there seems to be ongoing discussions on changing version number
while in the beta period (and which definitely requires initdb). Why
not changing system catalog during beta?:-)

I am not directly against that to be honest, but I'd expect Tom's
wraith showing up soon on this thread just by saying that. In the two
last releases, catalog bumps before beta2 because there were no other
choice. This issue is not really critical, just a stupid miss from me,
and we can live with this mistake as well.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier <michael.paquier@gmail.com> writes:

On Tue, Jun 21, 2016 at 9:58 AM, Tatsuo Ishii <ishii@postgresql.org> wrote:

Even there seems to be ongoing discussions on changing version number
while in the beta period (and which definitely requires initdb). Why
not changing system catalog during beta?:-)

I am not directly against that to be honest, but I'd expect Tom's
wraith showing up soon on this thread just by saying that. In the two
last releases, catalog bumps before beta2 because there were no other
choice. This issue is not really critical, just a stupid miss from me,
and we can live with this mistake as well.

Since pg_stat_wal_receiver is new in 9.6, it seems to me that it'd be
wise to try to get it right the first time. And it's not like we are
going to get to beta3 without another initdb --- we already know the
partial-aggregate design is broken and needs some more catalog changes.

What I would want to know is whether this specific change is actually a
good idea. In particular, I'm concerned about the possible security
implications of exposing primary_conninfo --- might it not contain a
password, for example?

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Jun 21, 2016 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Michael Paquier <michael.paquier@gmail.com> writes:

On Tue, Jun 21, 2016 at 9:58 AM, Tatsuo Ishii <ishii@postgresql.org> wrote:

Even there seems to be ongoing discussions on changing version number
while in the beta period (and which definitely requires initdb). Why
not changing system catalog during beta?:-)

I am not directly against that to be honest, but I'd expect Tom's
wraith showing up soon on this thread just by saying that. In the two
last releases, catalog bumps before beta2 because there were no other
choice. This issue is not really critical, just a stupid miss from me,
and we can live with this mistake as well.

Since pg_stat_wal_receiver is new in 9.6, it seems to me that it'd be
wise to try to get it right the first time. And it's not like we are
going to get to beta3 without another initdb --- we already know the
partial-aggregate design is broken and needs some more catalog changes.

Amen. That's a sufficient argument to slip this one into 9.6 then.

What I would want to know is whether this specific change is actually a
good idea. In particular, I'm concerned about the possible security
implications of exposing primary_conninfo --- might it not contain a
password, for example?

Yes it could, as a connection string, but we make the information of
this view only visible to superusers. For the others, that's just
NULL.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier <michael.paquier@gmail.com> writes:

On Tue, Jun 21, 2016 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

What I would want to know is whether this specific change is actually a
good idea. In particular, I'm concerned about the possible security
implications of exposing primary_conninfo --- might it not contain a
password, for example?

Yes it could, as a connection string, but we make the information of
this view only visible to superusers. For the others, that's just
NULL.

Well, that's okay for now, but I'm curious to hear Stephen Frost's
opinion on this. He's been on the warpath to decrease our dependence
on superuser-ness for protection purposes. Seems to me that having
one column in this view that is a lot more security-sensitive than
the others is likely to be an issue someday.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 6/20/16 10:29 PM, Tom Lane wrote:

What I would want to know is whether this specific change is actually a
good idea. In particular, I'm concerned about the possible security
implications of exposing primary_conninfo --- might it not contain a
password, for example?

That would have been my objection. This was also mentioned in the
context of moving recovery.conf settings to postgresql.conf, because
then the password would become visible in SHOW commands and the like.

We would need a way to put the password in a separate place, like a
primary_conn_password setting. Yes, you can already use .pgpass for
that, but since pg_basebackup -R will happily copy a password out of
.pgpass into recovery.conf, this makes accidentally leaking passwords
way too easy.

Alternatively or additionally, implement a way to strip passwords out of
conninfo information. libpq already has information about which
connection items are sensitive.

Needs more thought, in any case.

--
Peter Eisentraut http://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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 6/20/16 10:29 PM, Tom Lane wrote:

What I would want to know is whether this specific change is actually a
good idea. In particular, I'm concerned about the possible security
implications of exposing primary_conninfo --- might it not contain a
password, for example?

That would have been my objection. This was also mentioned in the
context of moving recovery.conf settings to postgresql.conf, because
then the password would become visible in SHOW commands and the like.

Alternatively or additionally, implement a way to strip passwords out of
conninfo information. libpq already has information about which
connection items are sensitive.

Yeah, I'd been wondering whether we could parse the conninfo string into
individual fields and then drop the password field. It's hard to see a
reason why this view needs to show passwords, since presumably everything
in it corresponds to successful connections --- if your password is wrong,
you aren't in it.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Jun 21, 2016 at 12:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 6/20/16 10:29 PM, Tom Lane wrote:

What I would want to know is whether this specific change is actually a
good idea. In particular, I'm concerned about the possible security
implications of exposing primary_conninfo --- might it not contain a
password, for example?

That would have been my objection. This was also mentioned in the
context of moving recovery.conf settings to postgresql.conf, because
then the password would become visible in SHOW commands and the like.

Alternatively or additionally, implement a way to strip passwords out of
conninfo information. libpq already has information about which
connection items are sensitive.

Yeah, I'd been wondering whether we could parse the conninfo string into
individual fields and then drop the password field. It's hard to see a
reason why this view needs to show passwords, since presumably everything
in it corresponds to successful connections --- if your password is wrong,
you aren't in it.

walreceiver.c does not have a direct dependency to libpq yet,
everything does through libpqwalreceiver. So a correct move would be
to unplug the low-level routines of PQconninfoParse into something in
src/common/, where both the backend and frontend could use it. And
then we use it to rebuild a connection string. Thoughts?
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Michael Paquier <michael.paquier@gmail.com> writes:

On Tue, Jun 21, 2016 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

What I would want to know is whether this specific change is actually a
good idea. In particular, I'm concerned about the possible security
implications of exposing primary_conninfo --- might it not contain a
password, for example?

Yes it could, as a connection string, but we make the information of
this view only visible to superusers. For the others, that's just
NULL.

Well, that's okay for now, but I'm curious to hear Stephen Frost's
opinion on this. He's been on the warpath to decrease our dependence
on superuser-ness for protection purposes. Seems to me that having
one column in this view that is a lot more security-sensitive than
the others is likely to be an issue someday.

Ugh. I would certainly rather not have yet another special, hard-coded,
bit of logic that magically makes things available to a superuser when
it's not available to regular users.

What that results in is the need to have a new default role to control
access to that column for the non-superuser case.

As for the password showing up, sorry, but we need a solution for *that*
regardless of the rest- the password shouldn't be exposed to anyone, nor
should it be sent and kept in the backend's memory for longer than
necessary. I'm not suggesting we've got that figured out already, but
that's where we should be trying to go.

Apologies, I've not followed this thread entirely, so these comments are
based only on what's quoted above. I'll try to read the complete thread
tomorrow.

Thanks!

Stephen

On Wed, Jun 22, 2016 at 12:04 AM, Stephen Frost <sfrost@snowman.net> wrote:

Ugh. I would certainly rather not have yet another special, hard-coded,
bit of logic that magically makes things available to a superuser when
it's not available to regular users.
What that results in is the need to have a new default role to control
access to that column for the non-superuser case.

OK, we could always update system_views.sql to revoke all rights from
public.. This ship has not sailed yet.

As for the password showing up, sorry, but we need a solution for *that*
regardless of the rest- the password shouldn't be exposed to anyone, nor
should it be sent and kept in the backend's memory for longer than
necessary. I'm not suggesting we've got that figured out already, but
that's where we should be trying to go.

This connection string is stored in shared memory in WalRcvData, and
that's the case for a couple of releases now, so it has already a high
footprint, though I agree that making that available at SQL level
makes it even worse. Looking at the code, as libpq does not link
directly to libpqcommon, I think that the cleanest solution is to do
something similar to wchar.c, aka split the parsing, deparsing
routines that we are going to use in a file located in src/backend/,
and then build libpq using it. Writing a patch for that would not be
that complicated. What is stored in WalRcvData is then the connection
string filtered of its password field, or we could just obfuscate it
with ***. It may still be useful to the user to know that a password
has been used.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Jun 22, 2016 at 10:51 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

This connection string is stored in shared memory in WalRcvData, and
that's the case for a couple of releases now, so it has already a high
footprint, though I agree that making that available at SQL level
makes it even worse. Looking at the code, as libpq does not link
directly to libpqcommon, I think that the cleanest solution is to do
something similar to wchar.c, aka split the parsing, deparsing
routines that we are going to use in a file located in src/backend/,
and then build libpq using it. Writing a patch for that would not be
that complicated. What is stored in WalRcvData is then the connection
string filtered of its password field, or we could just obfuscate it
with ***. It may still be useful to the user to know that a password
has been used.

I have been thinking more about that, and came up with the following
idea... We do not want to link libpq directly to the server, so let's
add a new routine to libpqwalreceiver that builds an obfuscated
connection string and let's have walreceiver.c save it in shared
memory. Then pg_stat_wal_receiver just makes use of this string. This
results in a rather light patch, proposed as attached. Connection URIs
get as well translated as connection strings via PQconninfo(), then
the new interface routine of libpqwalreceiver looks at dispchar to
determine if it should dump a field or not and obfuscates it with more
or less '****'.

Thoughts?
--
Michael

On Sun, Jun 19, 2016 at 05:56:12PM +0900, Michael Paquier wrote:

The new pg_stat_wal_receiver does not include primary_conninfo.
Looking at that now, it looks almost stupid not to include it...
Adding it now would require a catalog bump, so I am not sure if this
is acceptable at this stage for 9.6...

There is no value in avoiding catversion bumps at this time.

[Action required within 72 hours. This is a generic notification.]

The above-described topic is currently a PostgreSQL 9.6 open item. Alvaro,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
9.6 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1]/messages/by-id/20160527025039.GA447393@tornado.leadboat.com and send a status update within 72 hours of this
message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping 9.6rc1. Consequently, I will appreciate your
efforts toward speedy resolution. Thanks.

[1]: /messages/by-id/20160527025039.GA447393@tornado.leadboat.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Noah Misch wrote:

On Sun, Jun 19, 2016 at 05:56:12PM +0900, Michael Paquier wrote:

The new pg_stat_wal_receiver does not include primary_conninfo.
Looking at that now, it looks almost stupid not to include it...
Adding it now would require a catalog bump, so I am not sure if this
is acceptable at this stage for 9.6...

There is no value in avoiding catversion bumps at this time.

I'm looking at this problem now and will report back by Wed 29th EOB.

--
�lvaro Herrera http://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