Обсуждение: primary_conninfo missing from pg_stat_wal_receiver

Hi all,

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...

Regards,
-- 
Michael

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
https://www.postgresql.org/message-id/CAN1xZseiqNRh1ZE0seVk7UuHeWvFBEWG%2BFcW7Xfm_Nv%3Dd%2BfPGw%40mail.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

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

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

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

>> 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

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

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

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

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

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

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

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

* 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

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] 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] http://www.postgresql.org/message-id/20160527025039.GA447393@tornado.leadboat.com

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

Stephen Frost wrote:
> * 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.

FWIW we already have a superuser() check for the walsender stats view
since 9.1 -- see commit f88a6381.  To appease this we could create our
second predefined role that controls access to both
pg_stat_get_wal_senders and pg_stat_get_wal_receiver.  I don't think
my commit in 9.6 creates this problem, only exacerbates a pre-existing
one, but I also think it's fair to fix both cases for 9.6.

Not sure what to name the new predefined role though -- pg_wal_stats_reader?
(I don't suppose we want to create it to cover *any* future privileged
stats reads rather than just those WAL related, do we?)

> 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.

I suppose Michael's proposed patch to copy the conninfo obscuring the
password should be enough for this, but I'll go have a closer look.

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

Michael Paquier wrote:

> 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 '****'.

Seems a reasonable idea to me, but some details seem a bit strange:

* Why obfuscate debug options instead of skipping them?
* why not use PQExpBuffer?
* Why have the return param be an output argument instead of a plain return value? i.e. static char
*libpqrcv_get_conninfo(void).

On the security aspect of "conninfo" itself, which persists in shared
memory: do we absolutely need to keep that data?  In my reading of the
code, it's only used once to establish the initial connection to the
walsender, and then never afterwards.  We could remove the disclosure by
the simple expedient of overwriting that struct member with the
obfuscated one, right after establishing that connection.  Then we don't
need an additional struct member safe_conninfo.  Is there a reason why
this wouldn't work?

I have already edited the patch following some of these ideas.  Will
post a new version later.

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

On Wed, Jun 29, 2016 at 6:42 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Michael Paquier wrote:
>
>> 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 '****'.
>
> Seems a reasonable idea to me, but some details seem a bit strange:
>
> * Why obfuscate debug options instead of skipping them?

Those are hidden in postgres_fdw/ and 'D' marks options only used for
debugging purposes or options that should not be shown. That7s why I
did so.

> * why not use PQExpBuffer?

Yes, that would be better.

> * Why have the return param be an output argument instead of a plain
>   return value? i.e. static char *libpqrcv_get_conninfo(void).

Oh, yes. That's something I forgot to change. We cannot be completely
sure that the connstr will fit in MAXCONNINFO, so it makes little
sense to store the result in a pre-allocated string.

> On the security aspect of "conninfo" itself, which persists in shared
> memory: do we absolutely need to keep that data? In my reading of the
> code, it's only used once to establish the initial connection to the
> walsender, and then never afterwards.  We could remove the disclosure by
> the simple expedient of overwriting that struct member with the
> obfuscated one, right after establishing that connection.  Then we don't
> need an additional struct member safe_conninfo.  Is there a reason why
> this wouldn't work?

[Wait a minute...]
I don't see why that would not work. By reading the code we do not
reattempt a connection, and leave WalReceiverMain if there is a
disconnection.

> I have already edited the patch following some of these ideas.  Will
> post a new version later.

Cool, thanks.
-- 
Michael

Michael Paquier wrote:
> On Wed, Jun 29, 2016 at 6:42 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:

> > I have already edited the patch following some of these ideas.  Will
> > post a new version later.
>
> Cool, thanks.

Here it is.  I found it was annoying to maintain the function return
tupdesc in two places (pg_proc.h and the function code itself), so I
changed that too.

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

On Wed, Jun 29, 2016 at 12:23 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Michael Paquier wrote:
>> On Wed, Jun 29, 2016 at 6:42 AM, Alvaro Herrera
>> <alvherre@2ndquadrant.com> wrote:
>
>> > I have already edited the patch following some of these ideas.  Will
>> > post a new version later.
>>
>> Cool, thanks.
>
> Here it is.  I found it was annoying to maintain the function return
> tupdesc in two places (pg_proc.h and the function code itself), so I
> changed that too.

This looks globally fine to me. Good catch to handle NULL results of
walrcv_get_conninfo.

+       appendPQExpBuffer(&buf, "%s=%s ",
+                         conn_opt->keyword,
+                         obfuscate ? "********" : conn_opt->val)
This would add an extra space at the end of the string
unconditionally. What about checking if buf->len == 0, then fill buf
with "%s=%s", and " %s=%s" otherwise?

Do we want to do something for back-branches regarding the presence of
the connection string in shared memory? The only invasive point is the
addition of the interface routine to get back the obfuscated
connection string from libpqwalreceiver. That's a private interface in
the backend, but perhaps it would be a problem to change that in a
minor release?
-- 
Michael

On Wed, Jun 29, 2016 at 12:23 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Michael Paquier wrote:
>> On Wed, Jun 29, 2016 at 6:42 AM, Alvaro Herrera
>> <alvherre@2ndquadrant.com> wrote:
>
>> > I have already edited the patch following some of these ideas.  Will
>> > post a new version later.
>>
>> Cool, thanks.
>
> Here it is.  I found it was annoying to maintain the function return
> tupdesc in two places (pg_proc.h and the function code itself), so I
> changed that too.

ISTM that pg_stat_wal_receiver can return the security-sensitive fields
if it's viewed before walreceiver overwrites the conninfo in the shared memory
with the obfuscated one.

Regards,

-- 
Fujii Masao

Fujii Masao wrote:
> On Wed, Jun 29, 2016 at 12:23 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
> > Michael Paquier wrote:
> >> On Wed, Jun 29, 2016 at 6:42 AM, Alvaro Herrera
> >> <alvherre@2ndquadrant.com> wrote:
> >
> >> > I have already edited the patch following some of these ideas.  Will
> >> > post a new version later.
> >>
> >> Cool, thanks.
> >
> > Here it is.  I found it was annoying to maintain the function return
> > tupdesc in two places (pg_proc.h and the function code itself), so I
> > changed that too.
> 
> ISTM that pg_stat_wal_receiver can return the security-sensitive fields
> if it's viewed before walreceiver overwrites the conninfo in the shared memory
> with the obfuscated one.

Hmm, ouch.  Maybe we can set a flag once the conninfo has been
obfuscated, and put the function to sleep until the flag is set.

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

On Thu, Jun 30, 2016 at 2:50 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Fujii Masao wrote:
>> On Wed, Jun 29, 2016 at 12:23 PM, Alvaro Herrera
>> <alvherre@2ndquadrant.com> wrote:
>> > Michael Paquier wrote:
>> >> On Wed, Jun 29, 2016 at 6:42 AM, Alvaro Herrera
>> >> <alvherre@2ndquadrant.com> wrote:
>> >
>> >> > I have already edited the patch following some of these ideas.  Will
>> >> > post a new version later.
>> >>
>> >> Cool, thanks.
>> >
>> > Here it is.  I found it was annoying to maintain the function return
>> > tupdesc in two places (pg_proc.h and the function code itself), so I
>> > changed that too.
>>
>> ISTM that pg_stat_wal_receiver can return the security-sensitive fields
>> if it's viewed before walreceiver overwrites the conninfo in the shared memory
>> with the obfuscated one.
>
> Hmm, ouch.  Maybe we can set a flag once the conninfo has been
> obfuscated, and put the function to sleep until the flag is set.

Or what about making walreceiver instead of startup process read
primary_conninfo from the file?

Regards,

-- 
Fujii Masao

Fujii Masao wrote:
> On Thu, Jun 30, 2016 at 2:50 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
> > Fujii Masao wrote:
> >> On Wed, Jun 29, 2016 at 12:23 PM, Alvaro Herrera
> >> <alvherre@2ndquadrant.com> wrote:
> >> > Michael Paquier wrote:
> >> >> On Wed, Jun 29, 2016 at 6:42 AM, Alvaro Herrera
> >> >> <alvherre@2ndquadrant.com> wrote:
> >> >
> >> >> > I have already edited the patch following some of these ideas.  Will
> >> >> > post a new version later.
> >> >>
> >> >> Cool, thanks.
> >> >
> >> > Here it is.  I found it was annoying to maintain the function return
> >> > tupdesc in two places (pg_proc.h and the function code itself), so I
> >> > changed that too.
> >>
> >> ISTM that pg_stat_wal_receiver can return the security-sensitive fields
> >> if it's viewed before walreceiver overwrites the conninfo in the shared memory
> >> with the obfuscated one.
> >
> > Hmm, ouch.  Maybe we can set a flag once the conninfo has been
> > obfuscated, and put the function to sleep until the flag is set.
>
> Or what about making walreceiver instead of startup process read
> primary_conninfo from the file?

Yeah, that sounds smart.  I'm not sure it's a good fit for 9.6; what I
propose can be implemented in 10 lines, attached (wherein I also adopted
Michael's suggestion to get rid of the extra whitespace)

I propose to push this patch, closing the open item, and you can rework
on top -- I suppose you would completely remove the original conninfo
from shared memory and instead only copy the obfuscated version there
(and probably also remove the ready_to_display flag).  I think we'd need
to see the patch before deciding whether we want it in 9.6 or not,
keeping in mind that having the conninfo in shared memory is a
pre-existing problem, unrelated to the pgstats view new in 9.6.

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

Alvaro Herrera wrote:

> I propose to push this patch, closing the open item, and you can rework
> on top -- I suppose you would completely remove the original conninfo
> from shared memory and instead only copy the obfuscated version there
> (and probably also remove the ready_to_display flag).  I think we'd need
> to see the patch before deciding whether we want it in 9.6 or not,
> keeping in mind that having the conninfo in shared memory is a
> pre-existing problem, unrelated to the pgstats view new in 9.6.

Pushed this.  Feel free to tinker further with it, if you feel the need
to.

Regarding backpatching the clearing of shared memory, I'm inclined not
to.  If there is a real security concern there (I'm unsure what attack
are we protecting against), it may be better fixed by the approach
suggested by Fujii whereby the sensitive info is not ever published in
shared memory.

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

On Thu, Jun 30, 2016 at 6:01 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Alvaro Herrera wrote:
>
>> I propose to push this patch, closing the open item, and you can rework
>> on top -- I suppose you would completely remove the original conninfo
>> from shared memory and instead only copy the obfuscated version there
>> (and probably also remove the ready_to_display flag).  I think we'd need
>> to see the patch before deciding whether we want it in 9.6 or not,
>> keeping in mind that having the conninfo in shared memory is a
>> pre-existing problem, unrelated to the pgstats view new in 9.6.
>
> Pushed this.  Feel free to tinker further with it, if you feel the need
> to.
>
> Regarding backpatching the clearing of shared memory, I'm inclined not
> to.  If there is a real security concern there (I'm unsure what attack
> are we protecting against), it may be better fixed by the approach
> suggested by Fujii whereby the sensitive info is not ever published in
> shared memory.

Yes, this is not going to be pretty invasive anyway. The cleanest way
to handle things here would be to refactor a bit xlog.c
(xlogparams.c?) so as readRecoveryCommandFile is exposed in its own
file, and the recovery parameters are handled in a single structure,
which is the return result of the call. To reduce a bit the cruft in
xlog.c that would be nice anyway I guess.
-- 
Michael

Magnus Hagander <magnus@hagander.net> writes:
> There was also that (old) thread about making the recovery.conf parameters
> be general GUCs. I don't actually remember the consensus there, but diong
> that would certainly change how it's handled as well.

It strikes me that keeping a password embedded in the conninfo from being
exposed might be quite a bit harder/riskier if it became a GUC.  Something
to keep in mind if we ever try to make that change ...
        regards, tom lane

On Thu, Jun 30, 2016 at 6:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> There was also that (old) thread about making the recovery.conf parameters
>> be general GUCs. I don't actually remember the consensus there, but diong
>> that would certainly change how it's handled as well.
>
> It strikes me that keeping a password embedded in the conninfo from being
> exposed might be quite a bit harder/riskier if it became a GUC.  Something
> to keep in mind if we ever try to make that change ...

Exposing it in memory for a long time is an issue even if we have a
new GUC-flag to obfuscate the value in some cases..
-- 
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Thu, Jun 30, 2016 at 6:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It strikes me that keeping a password embedded in the conninfo from being
>> exposed might be quite a bit harder/riskier if it became a GUC.  Something
>> to keep in mind if we ever try to make that change ...

> Exposing it in memory for a long time is an issue even if we have a
> new GUC-flag to obfuscate the value in some cases..

Well, mumble ... I'm having a hard time understanding the threat model
we're guarding against there.  An attacker who can read process memory
can probably read the config file too.  I don't mind getting rid of the
in-memory copy if it's painless to do so, but I doubt that it's worth
any large amount of effort.
        regards, tom lane

On Thu, Jun 30, 2016 at 6:01 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Alvaro Herrera wrote:
>
>> I propose to push this patch, closing the open item, and you can rework
>> on top -- I suppose you would completely remove the original conninfo
>> from shared memory and instead only copy the obfuscated version there
>> (and probably also remove the ready_to_display flag).  I think we'd need
>> to see the patch before deciding whether we want it in 9.6 or not,
>> keeping in mind that having the conninfo in shared memory is a
>> pre-existing problem, unrelated to the pgstats view new in 9.6.
>
> Pushed this.

Thanks for pushing the patch!
But I found two problems in the patch you pushed.

(1)
ready_to_display flag must be reset to false when walreceiver dies.
Otherwise, pg_stat_wal_receiver can report the password (i.e.,
the problem that I reported upthread can happen) when walreceiver restarts
because ready_to_display flag is true from the beginning in that case.
But you forgot to reset the flag to false when walreceiver dies.

(2)
+retry:
+    SpinLockAcquire(&walrcv->mutex);
+    if (!walrcv->ready_to_display)
+    {
+        SpinLockRelease(&walrcv->mutex);
+        CHECK_FOR_INTERRUPTS();
+        pg_usleep(1000);
+        goto retry;
+    }
+    SpinLockRelease(&walrcv->mutex);

ISTM that we will never be able to get out of this loop if walreceiver
fails to connect to the master (e.g., password is wrong) after we enter
this loop.

Regards,

-- 
Fujii Masao

Fujii Masao wrote:
> On Thu, Jun 30, 2016 at 6:01 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
> > Alvaro Herrera wrote:
> >
> >> I propose to push this patch, closing the open item, and you can rework
> >> on top -- I suppose you would completely remove the original conninfo
> >> from shared memory and instead only copy the obfuscated version there
> >> (and probably also remove the ready_to_display flag).  I think we'd need
> >> to see the patch before deciding whether we want it in 9.6 or not,
> >> keeping in mind that having the conninfo in shared memory is a
> >> pre-existing problem, unrelated to the pgstats view new in 9.6.
> >
> > Pushed this.
> 
> Thanks for pushing the patch!
> But I found two problems in the patch you pushed.
> 
> (1)
> ready_to_display flag must be reset to false when walreceiver dies.
> Otherwise, pg_stat_wal_receiver can report the password (i.e.,
> the problem that I reported upthread can happen) when walreceiver restarts
> because ready_to_display flag is true from the beginning in that case.
> But you forgot to reset the flag to false when walreceiver dies.

Oops, you're right, since it's in shmem it doesn't get reset in the new
process.  Will fix.

> (2)
> +retry:
> +    SpinLockAcquire(&walrcv->mutex);
> +    if (!walrcv->ready_to_display)
> +    {
> +        SpinLockRelease(&walrcv->mutex);
> +        CHECK_FOR_INTERRUPTS();
> +        pg_usleep(1000);
> +        goto retry;
> +    }
> +    SpinLockRelease(&walrcv->mutex);
> 
> ISTM that we will never be able to get out of this loop if walreceiver
> fails to connect to the master (e.g., password is wrong) after we enter
> this loop.

Yeah, I thought that was OK.

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

On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> (2)
> +retry:
> +    SpinLockAcquire(&walrcv->mutex);
> +    if (!walrcv->ready_to_display)
> +    {
> +        SpinLockRelease(&walrcv->mutex);
> +        CHECK_FOR_INTERRUPTS();
> +        pg_usleep(1000);
> +        goto retry;
> +    }
> +    SpinLockRelease(&walrcv->mutex);
>
> ISTM that we will never be able to get out of this loop if walreceiver
> fails to connect to the master (e.g., password is wrong) after we enter
> this loop.

Wouldn't it be cleaner to just return an error here instead of retrying?
-- 
Michael

On Thu, Jun 30, 2016 at 9:30 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> (2)
>> +retry:
>> +    SpinLockAcquire(&walrcv->mutex);
>> +    if (!walrcv->ready_to_display)
>> +    {
>> +        SpinLockRelease(&walrcv->mutex);
>> +        CHECK_FOR_INTERRUPTS();
>> +        pg_usleep(1000);
>> +        goto retry;
>> +    }
>> +    SpinLockRelease(&walrcv->mutex);
>>
>> ISTM that we will never be able to get out of this loop if walreceiver
>> fails to connect to the master (e.g., password is wrong) after we enter
>> this loop.
>
> Wouldn't it be cleaner to just return an error here instead of retrying?

I prefer to return NULL. Now NULL is returned when walreceiver's pid is 0.
We can just change this logic so that NULL is returned pid is 0 OR the
flag is false.

Regards,

-- 
Fujii Masao

On Thu, Jun 30, 2016 at 9:35 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Thu, Jun 30, 2016 at 9:30 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> (2)
>>> +retry:
>>> +    SpinLockAcquire(&walrcv->mutex);
>>> +    if (!walrcv->ready_to_display)
>>> +    {
>>> +        SpinLockRelease(&walrcv->mutex);
>>> +        CHECK_FOR_INTERRUPTS();
>>> +        pg_usleep(1000);
>>> +        goto retry;
>>> +    }
>>> +    SpinLockRelease(&walrcv->mutex);
>>>
>>> ISTM that we will never be able to get out of this loop if walreceiver
>>> fails to connect to the master (e.g., password is wrong) after we enter
>>> this loop.
>>
>> Wouldn't it be cleaner to just return an error here instead of retrying?
>
> I prefer to return NULL. Now NULL is returned when walreceiver's pid is 0.
> We can just change this logic so that NULL is returned pid is 0 OR the
> flag is false.

OK, yes. That's indeed better this way. Need a patch?
-- 
Michael

Fujii Masao wrote:
> On Thu, Jun 30, 2016 at 9:30 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
> > On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

> >> ISTM that we will never be able to get out of this loop if walreceiver
> >> fails to connect to the master (e.g., password is wrong) after we enter
> >> this loop.
> >
> > Wouldn't it be cleaner to just return an error here instead of retrying?
> 
> I prefer to return NULL. Now NULL is returned when walreceiver's pid is 0.
> We can just change this logic so that NULL is returned pid is 0 OR the
> flag is false.

For the conninfo only, or for everything?

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

On Thu, Jun 30, 2016 at 9:41 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Fujii Masao wrote:
>> On Thu, Jun 30, 2016 at 9:30 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>> > On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>
>> >> ISTM that we will never be able to get out of this loop if walreceiver
>> >> fails to connect to the master (e.g., password is wrong) after we enter
>> >> this loop.
>> >
>> > Wouldn't it be cleaner to just return an error here instead of retrying?
>>
>> I prefer to return NULL. Now NULL is returned when walreceiver's pid is 0.
>> We can just change this logic so that NULL is returned pid is 0 OR the
>> flag is false.
>
> For the conninfo only, or for everything?

All of them. If this connstr is not ready for display, the WAL
receiver does not have a proper connection yet, so there is nothing
worth showing anyway to the user.
-- 
Michael

On Thu, Jun 30, 2016 at 10:12 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Jun 30, 2016 at 9:41 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> Fujii Masao wrote:
>>> On Thu, Jun 30, 2016 at 9:30 PM, Michael Paquier
>>> <michael.paquier@gmail.com> wrote:
>>> > On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>>> >> ISTM that we will never be able to get out of this loop if walreceiver
>>> >> fails to connect to the master (e.g., password is wrong) after we enter
>>> >> this loop.
>>> >
>>> > Wouldn't it be cleaner to just return an error here instead of retrying?
>>>
>>> I prefer to return NULL. Now NULL is returned when walreceiver's pid is 0.
>>> We can just change this logic so that NULL is returned pid is 0 OR the
>>> flag is false.
>>
>> For the conninfo only, or for everything?
>
> All of them. If this connstr is not ready for display, the WAL
> receiver does not have a proper connection yet, so there is nothing
> worth showing anyway to the user.

+1

Regards,

-- 
Fujii Masao

Fujii Masao wrote:
> On Thu, Jun 30, 2016 at 10:12 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
> > On Thu, Jun 30, 2016 at 9:41 PM, Alvaro Herrera
> > <alvherre@2ndquadrant.com> wrote:
> >> Fujii Masao wrote:
> >>> On Thu, Jun 30, 2016 at 9:30 PM, Michael Paquier
> >>> <michael.paquier@gmail.com> wrote:
> >>> > On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> >>
> >>> >> ISTM that we will never be able to get out of this loop if walreceiver
> >>> >> fails to connect to the master (e.g., password is wrong) after we enter
> >>> >> this loop.
> >>> >
> >>> > Wouldn't it be cleaner to just return an error here instead of retrying?
> >>>
> >>> I prefer to return NULL. Now NULL is returned when walreceiver's pid is 0.
> >>> We can just change this logic so that NULL is returned pid is 0 OR the
> >>> flag is false.
> >>
> >> For the conninfo only, or for everything?
> >
> > All of them. If this connstr is not ready for display, the WAL
> > receiver does not have a proper connection yet, so there is nothing
> > worth showing anyway to the user.
> 
> +1

slotname seems worth showing.  And if this process just started after
some other process was already receiving, then the LSN fields surely can
have useful data too.

Also, actually, I see no reason for the conninfo to be shown differently
regardless of a connection being already established.  If we show the
conninfo that the server is trying to use, it might be easier to
diagnose a problem.  In short, I think this is all misconceived (mea
culpa) and that we should have two conninfo members in that struct as
initially proposed, one obfuscated and the other not.

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

On Thu, Jun 30, 2016 at 11:24 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Also, actually, I see no reason for the conninfo to be shown differently
> regardless of a connection being already established.  If we show the
> conninfo that the server is trying to use, it might be easier to
> diagnose a problem.  In short, I think this is all misconceived (mea
> culpa) and that we should have two conninfo members in that struct as
> initially proposed, one obfuscated and the other not.

If the conninfo is leaking an incorrect password, say it has only a
couple of characters of difference with the real one, we'd still leak
information. That's not good IMO based on the concerns raised on this
thread. I'd just mark all the fields as NULL in this case and move on.
This way the code keeps being simple, and having this information
means that the WAL receiver is correctly working. The window where the
information of a failed connection is rather limited as well, the WAL
receiver process shuts down immediately and would reset its PID to 0,
hiding the information anyway.
-- 
Michael

Michael Paquier wrote:
> On Thu, Jun 30, 2016 at 11:24 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
> > Also, actually, I see no reason for the conninfo to be shown differently
> > regardless of a connection being already established.  If we show the
> > conninfo that the server is trying to use, it might be easier to
> > diagnose a problem.  In short, I think this is all misconceived (mea
> > culpa) and that we should have two conninfo members in that struct as
> > initially proposed, one obfuscated and the other not.
> 
> If the conninfo is leaking an incorrect password, say it has only a
> couple of characters of difference with the real one, we'd still leak
> information.

No, I don't mean to leak any password.  It would still be obfuscated,
but all other details would be there (anything with default values).

> The window where the information of a failed connection is rather
> limited as well, the WAL receiver process shuts down immediately and
> would reset its PID to 0, hiding the information anyway.

Some of the details are set by the startup process, such as the start
LSN etc, not the walreceiver.  Only the PID is reset AFAICS.

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

On Fri, Jul 1, 2016 at 8:35 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Michael Paquier wrote:
>> On Thu, Jun 30, 2016 at 11:24 PM, Alvaro Herrera
>> <alvherre@2ndquadrant.com> wrote:
>> > Also, actually, I see no reason for the conninfo to be shown differently
>> > regardless of a connection being already established.  If we show the
>> > conninfo that the server is trying to use, it might be easier to
>> > diagnose a problem.  In short, I think this is all misconceived (mea
>> > culpa) and that we should have two conninfo members in that struct as
>> > initially proposed, one obfuscated and the other not.
>>
>> If the conninfo is leaking an incorrect password, say it has only a
>> couple of characters of difference with the real one, we'd still leak
>> information.
>
> No, I don't mean to leak any password.  It would still be obfuscated,
> but all other details would be there (anything with default values).

OK. There is no need to use two fields by the way. The WAL receiver
makes no attempts to reconnect with the same string and leaves immediately
should a connection fail.

>> The window where the information of a failed connection is rather
>> limited as well, the WAL receiver process shuts down immediately and
>> would reset its PID to 0, hiding the information anyway.
>
> Some of the details are set by the startup process, such as the start
> LSN etc, not the walreceiver.  Only the PID is reset AFAICS.

Yeah, I know. Now my opinion regarding this view is that we should
show information about a currently-working WAL receiver, and that it
has nothing to do with reporting information of its previous startup state.
That's more consistent with the WAL sender.
-- 
Michael

Michael Paquier wrote:
> On Fri, Jul 1, 2016 at 8:35 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > Michael Paquier wrote:
> >> On Thu, Jun 30, 2016 at 11:24 PM, Alvaro Herrera
> >> <alvherre@2ndquadrant.com> wrote:
> >> > Also, actually, I see no reason for the conninfo to be shown differently
> >> > regardless of a connection being already established.  If we show the
> >> > conninfo that the server is trying to use, it might be easier to
> >> > diagnose a problem.  In short, I think this is all misconceived (mea
> >> > culpa) and that we should have two conninfo members in that struct as
> >> > initially proposed, one obfuscated and the other not.
> >>
> >> If the conninfo is leaking an incorrect password, say it has only a
> >> couple of characters of difference with the real one, we'd still leak
> >> information.
> >
> > No, I don't mean to leak any password.  It would still be obfuscated,
> > but all other details would be there (anything with default values).
> 
> OK. There is no need to use two fields by the way. The WAL receiver
> makes no attempts to reconnect with the same string and leaves immediately
> should a connection fail.

Yes, but the question is what happens if somebody queries before
walreceiver attempts to connect, no?  That's the case where the current
code loops.

> >> The window where the information of a failed connection is rather
> >> limited as well, the WAL receiver process shuts down immediately and
> >> would reset its PID to 0, hiding the information anyway.
> >
> > Some of the details are set by the startup process, such as the start
> > LSN etc, not the walreceiver.  Only the PID is reset AFAICS.
> 
> Yeah, I know. Now my opinion regarding this view is that we should
> show information about a currently-working WAL receiver, and that it
> has nothing to do with reporting information of its previous startup state.
> That's more consistent with the WAL sender.

Okay, that argument I buy.

I suppose this function/view should report no row at all if there is no
wal receiver connected, rather than a view with nulls.

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

On Fri, Jul 1, 2016 at 8:48 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Michael Paquier wrote:
>> Yeah, I know. Now my opinion regarding this view is that we should
>> show information about a currently-working WAL receiver, and that it
>> has nothing to do with reporting information of its previous startup state.
>> That's more consistent with the WAL sender.
>
> Okay, that argument I buy.
>
> I suppose this function/view should report no row at all if there is no
> wal receiver connected, rather than a view with nulls.

The function returns PG_RETURN_NULL() so as we don't have to use a
SRF, and the view checks for IS NOT NULL, so there would be no rows
popping up.
-- 
Michael

On Fri, Jul 1, 2016 at 8:50 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Jul 1, 2016 at 8:48 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> Michael Paquier wrote:
>>> Yeah, I know. Now my opinion regarding this view is that we should
>>> show information about a currently-working WAL receiver, and that it
>>> has nothing to do with reporting information of its previous startup state.
>>> That's more consistent with the WAL sender.
>>
>> Okay, that argument I buy.
>>
>> I suppose this function/view should report no row at all if there is no
>> wal receiver connected, rather than a view with nulls.
>
> The function returns PG_RETURN_NULL() so as we don't have to use a
> SRF, and the view checks for IS NOT NULL, so there would be no rows
> popping up.

In short, I would just go with the attached and call it a day.
--
Michael

Michael Paquier wrote:
> On Fri, Jul 1, 2016 at 8:50 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:

> >> Okay, that argument I buy.
> >>
> >> I suppose this function/view should report no row at all if there is no
> >> wal receiver connected, rather than a view with nulls.
> >
> > The function returns PG_RETURN_NULL() so as we don't have to use a
> > SRF, and the view checks for IS NOT NULL, so there would be no rows
> > popping up.
> 
> In short, I would just go with the attached and call it a day.

Done, thanks.

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

On Sat, Jul 2, 2016 at 2:56 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Michael Paquier wrote:
>> On Fri, Jul 1, 2016 at 8:50 AM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>
>> >> Okay, that argument I buy.
>> >>
>> >> I suppose this function/view should report no row at all if there is no
>> >> wal receiver connected, rather than a view with nulls.
>> >
>> > The function returns PG_RETURN_NULL() so as we don't have to use a
>> > SRF, and the view checks for IS NOT NULL, so there would be no rows
>> > popping up.
>>
>> In short, I would just go with the attached and call it a day.
>
> Done, thanks.

Thanks. I have noticed that the item was still in CLOSE_WAIT, so I
have moved it to the section of resolved items.
-- 
Michael

On Mon, Jul 4, 2016 at 12:40 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Sat, Jul 2, 2016 at 2:56 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> Michael Paquier wrote:
>>> On Fri, Jul 1, 2016 at 8:50 AM, Michael Paquier
>>> <michael.paquier@gmail.com> wrote:
>>
>>> >> Okay, that argument I buy.
>>> >>
>>> >> I suppose this function/view should report no row at all if there is no
>>> >> wal receiver connected, rather than a view with nulls.
>>> >
>>> > The function returns PG_RETURN_NULL() so as we don't have to use a
>>> > SRF, and the view checks for IS NOT NULL, so there would be no rows
>>> > popping up.
>>>
>>> In short, I would just go with the attached and call it a day.
>>
>> Done, thanks.

Thanks!

I have one question; why do we call the column "conn_info" instead of
"conninfo" which is basically used in other places? "conninfo" is better to me.

Regards,

-- 
Fujii Masao

On Wed, Jul 6, 2016 at 7:34 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> I have one question; why do we call the column "conn_info" instead of
> "conninfo" which is basically used in other places? "conninfo" is better to me.

No real reason for one or the other to be honest. If you want to
change it you could just apply the attached.
--
Michael

Michael Paquier wrote:
> On Wed, Jul 6, 2016 at 7:34 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> > I have one question; why do we call the column "conn_info" instead of
> > "conninfo" which is basically used in other places? "conninfo" is better to me.
> 
> No real reason for one or the other to be honest. If you want to
> change it you could just apply the attached.

I was of two minds myself, and found no reason to change conn_info, so I
decided to keep what was submitted.  If you want to change it, I'm not
opposed.

Don't forget to bump catversion.

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

All,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Michael Paquier wrote:
> > On Wed, Jul 6, 2016 at 7:34 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> > > I have one question; why do we call the column "conn_info" instead of
> > > "conninfo" which is basically used in other places? "conninfo" is better to me.
> >
> > No real reason for one or the other to be honest. If you want to
> > change it you could just apply the attached.
>
> I was of two minds myself, and found no reason to change conn_info, so I
> decided to keep what was submitted.  If you want to change it, I'm not
> opposed.
>
> Don't forget to bump catversion.

'conninfo' certainly seems to be more commonly used and I believe is
what was agreed to up-thread.

Thanks!

Stephen

On Thu, Jul 7, 2016 at 4:43 AM, Stephen Frost <sfrost@snowman.net> wrote:
> All,
>
> * Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
>> Michael Paquier wrote:
>> > On Wed, Jul 6, 2016 at 7:34 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> > > I have one question; why do we call the column "conn_info" instead of
>> > > "conninfo" which is basically used in other places? "conninfo" is better to me.
>> >
>> > No real reason for one or the other to be honest. If you want to
>> > change it you could just apply the attached.
>>
>> I was of two minds myself, and found no reason to change conn_info, so I
>> decided to keep what was submitted.  If you want to change it, I'm not
>> opposed.
>>
>> Don't forget to bump catversion.
>
> 'conninfo' certainly seems to be more commonly used and I believe is
> what was agreed to up-thread.

+1. So since no one objects to change the column name,
I applied Michael's patch. Thanks!

Regards,

-- 
Fujii Masao

On Thu, Jun 30, 2016 at 10:24 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Also, actually, I see no reason for the conninfo to be shown differently
> regardless of a connection being already established.  If we show the
> conninfo that the server is trying to use, it might be easier to
> diagnose a problem.  In short, I think this is all misconceived (mea
> culpa) and that we should have two conninfo members in that struct as
> initially proposed, one obfuscated and the other not.

Seriously!

The whole problem here is being created by trying to use the same
field for two different purposes:

1. The string that should actually be used for connections.
2. The sanitized version that should be exposed to the user.

If you try to use the same variable to store two different values,
both bugs and confusion may result.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company