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
--
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 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
Attachments:
wal-receiver-conninfo.patchinvalid/octet-stream; name=wal-receiver-conninfo.patchDownload
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 272c02f..8634b39 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -681,7 +681,8 @@ CREATE VIEW pg_stat_wal_receiver AS
s.last_msg_receipt_time,
s.latest_end_lsn,
s.latest_end_time,
- s.slot_name
+ s.slot_name,
+ s.conn_info
FROM pg_stat_get_wal_receiver() s
WHERE s.pid IS NOT NULL;
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index ce311cb..ea3a01d 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -1308,7 +1308,7 @@ WalRcvGetStateString(WalRcvState state)
Datum
pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
{
-#define PG_STAT_GET_WAL_RECEIVER_COLS 11
+#define PG_STAT_GET_WAL_RECEIVER_COLS 12
TupleDesc tupdesc;
Datum values[PG_STAT_GET_WAL_RECEIVER_COLS];
bool nulls[PG_STAT_GET_WAL_RECEIVER_COLS];
@@ -1323,6 +1323,7 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
XLogRecPtr latest_end_lsn;
TimestampTz latest_end_time;
char *slotname;
+ char *conninfo;
/* No WAL receiver, just return a tuple with NULL values */
if (walrcv->pid == 0)
@@ -1356,6 +1357,8 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
TIMESTAMPTZOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 11, "slot_name",
TEXTOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 12, "conn_info",
+ TEXTOID, -1, 0);
BlessTupleDesc(tupdesc);
@@ -1371,6 +1374,7 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
latest_end_lsn = walrcv->latestWalEnd;
latest_end_time = walrcv->latestWalEndTime;
slotname = pstrdup(walrcv->slotname);
+ conninfo = pstrdup(walrcv->conninfo);
SpinLockRelease(&walrcv->mutex);
/* Fetch values */
@@ -1418,6 +1422,10 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
nulls[10] = true;
else
values[10] = CStringGetTextDatum(slotname);
+ if (*conninfo == '\0')
+ nulls[11] = true;
+ else
+ values[11] = CStringGetTextDatum(conninfo);
}
/* Returns the record as Datum */
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
Attachments:
wal-receiver-conninfo-v2.patchinvalid/octet-stream; name=wal-receiver-conninfo-v2.patchDownload
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index bd7bb77..422b2d0 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1302,6 +1302,11 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
<entry><type>text</></entry>
<entry>Replication slot name used by this WAL receiver</entry>
</row>
+ <row>
+ <entry><structfield>conn_info</></entry>
+ <entry><type>text</></entry>
+ <entry>Connection string used by this WAL receiver</entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 272c02f..f52de3a 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -681,7 +681,8 @@ CREATE VIEW pg_stat_wal_receiver AS
s.last_msg_receipt_time,
s.latest_end_lsn,
s.latest_end_time,
- s.slot_name
+ s.slot_name,
+ s.conn_info
FROM pg_stat_get_wal_receiver() s
WHERE s.pid IS NOT NULL;
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index ce311cb..ea3a01d 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -1308,7 +1308,7 @@ WalRcvGetStateString(WalRcvState state)
Datum
pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
{
-#define PG_STAT_GET_WAL_RECEIVER_COLS 11
+#define PG_STAT_GET_WAL_RECEIVER_COLS 12
TupleDesc tupdesc;
Datum values[PG_STAT_GET_WAL_RECEIVER_COLS];
bool nulls[PG_STAT_GET_WAL_RECEIVER_COLS];
@@ -1323,6 +1323,7 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
XLogRecPtr latest_end_lsn;
TimestampTz latest_end_time;
char *slotname;
+ char *conninfo;
/* No WAL receiver, just return a tuple with NULL values */
if (walrcv->pid == 0)
@@ -1356,6 +1357,8 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
TIMESTAMPTZOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 11, "slot_name",
TEXTOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 12, "conn_info",
+ TEXTOID, -1, 0);
BlessTupleDesc(tupdesc);
@@ -1371,6 +1374,7 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
latest_end_lsn = walrcv->latestWalEnd;
latest_end_time = walrcv->latestWalEndTime;
slotname = pstrdup(walrcv->slotname);
+ conninfo = pstrdup(walrcv->conninfo);
SpinLockRelease(&walrcv->mutex);
/* Fetch values */
@@ -1418,6 +1422,10 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
nulls[10] = true;
else
values[10] = CStringGetTextDatum(slotname);
+ if (*conninfo == '\0')
+ nulls[11] = true;
+ else
+ values[11] = CStringGetTextDatum(conninfo);
}
/* Returns the record as Datum */
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 6a65e77..623cdb9 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -2746,7 +2746,7 @@ DATA(insert OID = 3318 ( pg_stat_get_progress_info PGNSP PGUID 12 1 100 0 0
DESCR("statistics: information about progress of backends running maintenance command");
DATA(insert OID = 3099 ( pg_stat_get_wal_senders PGNSP PGUID 12 1 10 0 0 f f f f f t s r 0 0 2249 "" "{23,25,3220,3220,3220,3220,23,25}" "{o,o,o,o,o,o,o,o}" "{pid,state,sent_location,write_location,flush_location,replay_location,sync_priority,sync_state}" _null_ _null_ pg_stat_get_wal_senders _null_ _null_ _null_ ));
DESCR("statistics: information about currently active replication");
-DATA(insert OID = 3317 ( pg_stat_get_wal_receiver PGNSP PGUID 12 1 0 0 0 f f f f f f s r 0 0 2249 "" "{23,25,3220,23,3220,23,1184,1184,3220,1184,25}" "{o,o,o,o,o,o,o,o,o,o,o}" "{pid,status,receive_start_lsn,receive_start_tli,received_lsn,received_tli,last_msg_send_time,last_msg_receipt_time,latest_end_lsn,latest_end_time,slot_name}" _null_ _null_ pg_stat_get_wal_receiver _null_ _null_ _null_ ));
+DATA(insert OID = 3317 ( pg_stat_get_wal_receiver PGNSP PGUID 12 1 0 0 0 f f f f f f s r 0 0 2249 "" "{23,25,3220,23,3220,23,1184,1184,3220,1184,25,25}" "{o,o,o,o,o,o,o,o,o,o,o,o}" "{pid,status,receive_start_lsn,receive_start_tli,received_lsn,received_tli,last_msg_send_time,last_msg_receipt_time,latest_end_lsn,latest_end_time,slot_name,conn_info}" _null_ _null_ pg_stat_get_wal_receiver _null_ _null_ _null_ ));
DESCR("statistics: information about WAL receiver");
DATA(insert OID = 2026 ( pg_backend_pid PGNSP PGUID 12 1 0 0 0 f f f f t f s r 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_backend_pid _null_ _null_ _null_ ));
DESCR("statistics: current backend PID");
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
Attachments:
wal-receiver-conninfo-v3.patchinvalid/octet-stream; name=wal-receiver-conninfo-v3.patchDownload
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index bd7bb77..8a95f42 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1302,6 +1302,13 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
<entry><type>text</></entry>
<entry>Replication slot name used by this WAL receiver</entry>
</row>
+ <row>
+ <entry><structfield>conn_info</></entry>
+ <entry><type>text</></entry>
+ <entry>Connection string used by this WAL receiver. This connection
+ string obfuscates any sensitive parameters like password values.
+ </entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 272c02f..f52de3a 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -681,7 +681,8 @@ CREATE VIEW pg_stat_wal_receiver AS
s.last_msg_receipt_time,
s.latest_end_lsn,
s.latest_end_time,
- s.slot_name
+ s.slot_name,
+ s.conn_info
FROM pg_stat_get_wal_receiver() s
WHERE s.pid IS NOT NULL;
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index b61e39d..6f9d087 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -47,6 +47,7 @@ static char *recvBuf = NULL;
/* Prototypes for interface functions */
static void libpqrcv_connect(char *conninfo);
+static void libpqrcv_get_conninfo(char **safe_connstr);
static void libpqrcv_identify_system(TimeLineID *primary_tli);
static void libpqrcv_readtimelinehistoryfile(TimeLineID tli, char **filename, char **content, int *len);
static bool libpqrcv_startstreaming(TimeLineID tli, XLogRecPtr startpoint,
@@ -74,6 +75,7 @@ _PG_init(void)
walrcv_disconnect != NULL)
elog(ERROR, "libpqwalreceiver already loaded");
walrcv_connect = libpqrcv_connect;
+ walrcv_get_conninfo = libpqrcv_get_conninfo;
walrcv_identify_system = libpqrcv_identify_system;
walrcv_readtimelinehistoryfile = libpqrcv_readtimelinehistoryfile;
walrcv_startstreaming = libpqrcv_startstreaming;
@@ -118,6 +120,66 @@ libpqrcv_connect(char *conninfo)
}
/*
+ * Return an obfuscated connection string to caller, this field is aimed at
+ * being made visible to users and hides any sensitive data like password
+ * data.
+ */
+static void
+libpqrcv_get_conninfo(char **safe_conninfo)
+{
+ PQconninfoOption *conn_opts = NULL;
+ PQconninfoOption *conn_opt;
+
+ Assert(streamConn != NULL);
+
+ *safe_conninfo = NULL;
+
+ conn_opts = PQconninfo(streamConn);
+
+ if (conn_opts == NULL)
+ ereport(ERROR,
+ (errmsg("could not parse connection string: %s",
+ PQerrorMessage(streamConn))));
+
+ /* Rebuild the connection string */
+ for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
+ {
+ bool obfuscate = false;
+
+ /* Skip any user-sensitive data */
+ if (strchr(conn_opt->dispchar, '*') || strchr(conn_opt->dispchar, 'D'))
+ obfuscate = true;
+
+ if (conn_opt->val != NULL && conn_opt->val[0] != '\0')
+ {
+ int len = strlen(conn_opt->val) + strlen(conn_opt->keyword) + 1;
+ bool first_loop = *safe_conninfo == NULL;
+
+ if (first_loop)
+ {
+ len++; /* '=' */
+ *safe_conninfo = (char *) palloc(len);
+ sprintf(*safe_conninfo, "%s=%s",
+ conn_opt->keyword,
+ obfuscate ? "*******" : conn_opt->val);
+ }
+ else
+ {
+ len += 2; /* '=' and space */
+ *safe_conninfo = (char *) repalloc(*safe_conninfo,
+ strlen(*safe_conninfo) + len);
+ sprintf(*safe_conninfo, "%s %s=%s",
+ *safe_conninfo,
+ conn_opt->keyword,
+ obfuscate ? "*******" : conn_opt->val);
+ }
+ }
+ }
+
+ PQconninfoFree(conn_opts);
+}
+
+/*
* Check that primary's system identifier matches ours, and fetch the current
* timeline ID of the primary.
*/
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index ce311cb..699ce33 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -75,6 +75,7 @@ bool hot_standby_feedback;
/* libpqreceiver hooks to these when loaded */
walrcv_connect_type walrcv_connect = NULL;
+walrcv_get_conninfo_type walrcv_get_conninfo = NULL;
walrcv_identify_system_type walrcv_identify_system = NULL;
walrcv_startstreaming_type walrcv_startstreaming = NULL;
walrcv_endstreaming_type walrcv_endstreaming = NULL;
@@ -192,6 +193,7 @@ void
WalReceiverMain(void)
{
char conninfo[MAXCONNINFO];
+ char *safe_conninfo;
char slotname[NAMEDATALEN];
XLogRecPtr startpoint;
TimeLineID startpointTLI;
@@ -282,7 +284,9 @@ WalReceiverMain(void)
/* Load the libpq-specific functions */
load_file("libpqwalreceiver", false);
- if (walrcv_connect == NULL || walrcv_startstreaming == NULL ||
+ if (walrcv_connect == NULL ||
+ walrcv_get_conninfo == NULL ||
+ walrcv_startstreaming == NULL ||
walrcv_endstreaming == NULL ||
walrcv_identify_system == NULL ||
walrcv_readtimelinehistoryfile == NULL ||
@@ -304,6 +308,13 @@ WalReceiverMain(void)
walrcv_connect(conninfo);
DisableWalRcvImmediateExit();
+ /* Save connection string visible to user */
+ walrcv_get_conninfo(&safe_conninfo);
+ SpinLockAcquire(&walrcv->mutex);
+ strlcpy((char *) walrcv->safe_conninfo, safe_conninfo, MAXCONNINFO);
+ SpinLockRelease(&walrcv->mutex);
+ pfree(safe_conninfo);
+
first_stream = true;
for (;;)
{
@@ -1308,7 +1319,7 @@ WalRcvGetStateString(WalRcvState state)
Datum
pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
{
-#define PG_STAT_GET_WAL_RECEIVER_COLS 11
+#define PG_STAT_GET_WAL_RECEIVER_COLS 12
TupleDesc tupdesc;
Datum values[PG_STAT_GET_WAL_RECEIVER_COLS];
bool nulls[PG_STAT_GET_WAL_RECEIVER_COLS];
@@ -1323,6 +1334,7 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
XLogRecPtr latest_end_lsn;
TimestampTz latest_end_time;
char *slotname;
+ char *conninfo;
/* No WAL receiver, just return a tuple with NULL values */
if (walrcv->pid == 0)
@@ -1356,6 +1368,8 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
TIMESTAMPTZOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 11, "slot_name",
TEXTOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 12, "conn_info",
+ TEXTOID, -1, 0);
BlessTupleDesc(tupdesc);
@@ -1371,6 +1385,7 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
latest_end_lsn = walrcv->latestWalEnd;
latest_end_time = walrcv->latestWalEndTime;
slotname = pstrdup(walrcv->slotname);
+ conninfo = pstrdup(walrcv->safe_conninfo);
SpinLockRelease(&walrcv->mutex);
/* Fetch values */
@@ -1418,6 +1433,10 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
nulls[10] = true;
else
values[10] = CStringGetTextDatum(slotname);
+ if (*conninfo == '\0')
+ nulls[11] = true;
+ else
+ values[11] = CStringGetTextDatum(conninfo);
}
/* Returns the record as Datum */
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index f441025..d92c05e 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -2746,7 +2746,7 @@ DATA(insert OID = 3318 ( pg_stat_get_progress_info PGNSP PGUID 12 1 100 0 0
DESCR("statistics: information about progress of backends running maintenance command");
DATA(insert OID = 3099 ( pg_stat_get_wal_senders PGNSP PGUID 12 1 10 0 0 f f f f f t s r 0 0 2249 "" "{23,25,3220,3220,3220,3220,23,25}" "{o,o,o,o,o,o,o,o}" "{pid,state,sent_location,write_location,flush_location,replay_location,sync_priority,sync_state}" _null_ _null_ pg_stat_get_wal_senders _null_ _null_ _null_ ));
DESCR("statistics: information about currently active replication");
-DATA(insert OID = 3317 ( pg_stat_get_wal_receiver PGNSP PGUID 12 1 0 0 0 f f f f f f s r 0 0 2249 "" "{23,25,3220,23,3220,23,1184,1184,3220,1184,25}" "{o,o,o,o,o,o,o,o,o,o,o}" "{pid,status,receive_start_lsn,receive_start_tli,received_lsn,received_tli,last_msg_send_time,last_msg_receipt_time,latest_end_lsn,latest_end_time,slot_name}" _null_ _null_ pg_stat_get_wal_receiver _null_ _null_ _null_ ));
+DATA(insert OID = 3317 ( pg_stat_get_wal_receiver PGNSP PGUID 12 1 0 0 0 f f f f f f s r 0 0 2249 "" "{23,25,3220,23,3220,23,1184,1184,3220,1184,25,25}" "{o,o,o,o,o,o,o,o,o,o,o,o}" "{pid,status,receive_start_lsn,receive_start_tli,received_lsn,received_tli,last_msg_send_time,last_msg_receipt_time,latest_end_lsn,latest_end_time,slot_name,conn_info}" _null_ _null_ pg_stat_get_wal_receiver _null_ _null_ _null_ ));
DESCR("statistics: information about WAL receiver");
DATA(insert OID = 2026 ( pg_backend_pid PGNSP PGUID 12 1 0 0 0 f f f f t f s r 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_backend_pid _null_ _null_ _null_ ));
DESCR("statistics: current backend PID");
diff --git a/src/include/replication/walreceiver.h b/src/include/replication/walreceiver.h
index c87e7a8..7e793b0 100644
--- a/src/include/replication/walreceiver.h
+++ b/src/include/replication/walreceiver.h
@@ -105,6 +105,12 @@ typedef struct
char conninfo[MAXCONNINFO];
/*
+ * Obfuscated connection string; is made visible to users and hides any
+ * sensitive data.
+ */
+ char safe_conninfo[MAXCONNINFO];
+
+ /*
* replication slot name; is also used for walreceiver to connect with the
* primary
*/
@@ -133,6 +139,9 @@ extern WalRcvData *WalRcv;
typedef void (*walrcv_connect_type) (char *conninfo);
extern PGDLLIMPORT walrcv_connect_type walrcv_connect;
+typedef void (*walrcv_get_conninfo_type) (char **safe_conninfo);
+extern PGDLLIMPORT walrcv_get_conninfo_type walrcv_get_conninfo;
+
typedef void (*walrcv_identify_system_type) (TimeLineID *primary_tli);
extern PGDLLIMPORT walrcv_identify_system_type walrcv_identify_system;
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
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
--
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 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
--
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 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
--
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 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
Attachments:
wal-receiver-conninfo-v4.patchtext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index bd7bb77..a8b8bb0 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1302,6 +1302,14 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
<entry><type>text</></entry>
<entry>Replication slot name used by this WAL receiver</entry>
</row>
+ <row>
+ <entry><structfield>conn_info</></entry>
+ <entry><type>text</></entry>
+ <entry>
+ Connection string used by this WAL receiver,
+ with security-sensitive fields obfuscated.
+ </entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 272c02f..f52de3a 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -681,7 +681,8 @@ CREATE VIEW pg_stat_wal_receiver AS
s.last_msg_receipt_time,
s.latest_end_lsn,
s.latest_end_time,
- s.slot_name
+ s.slot_name,
+ s.conn_info
FROM pg_stat_get_wal_receiver() s
WHERE s.pid IS NOT NULL;
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index b61e39d..2a10c56 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -20,6 +20,7 @@
#include <sys/time.h>
#include "libpq-fe.h"
+#include "pqexpbuffer.h"
#include "access/xlog.h"
#include "miscadmin.h"
#include "replication/walreceiver.h"
@@ -47,6 +48,7 @@ static char *recvBuf = NULL;
/* Prototypes for interface functions */
static void libpqrcv_connect(char *conninfo);
+static char *libpqrcv_get_conninfo(void);
static void libpqrcv_identify_system(TimeLineID *primary_tli);
static void libpqrcv_readtimelinehistoryfile(TimeLineID tli, char **filename, char **content, int *len);
static bool libpqrcv_startstreaming(TimeLineID tli, XLogRecPtr startpoint,
@@ -74,6 +76,7 @@ _PG_init(void)
walrcv_disconnect != NULL)
elog(ERROR, "libpqwalreceiver already loaded");
walrcv_connect = libpqrcv_connect;
+ walrcv_get_conninfo = libpqrcv_get_conninfo;
walrcv_identify_system = libpqrcv_identify_system;
walrcv_readtimelinehistoryfile = libpqrcv_readtimelinehistoryfile;
walrcv_startstreaming = libpqrcv_startstreaming;
@@ -118,6 +121,54 @@ libpqrcv_connect(char *conninfo)
}
/*
+ * Return a user-displayable conninfo string. Any security-sensitive fields
+ * are obfuscated.
+ */
+static char *
+libpqrcv_get_conninfo(void)
+{
+ PQconninfoOption *conn_opts;
+ PQconninfoOption *conn_opt;
+ PQExpBufferData buf;
+ char *retval;
+
+ Assert(streamConn != NULL);
+
+ initPQExpBuffer(&buf);
+ conn_opts = PQconninfo(streamConn);
+
+ if (conn_opts == NULL)
+ ereport(ERROR,
+ (errmsg("could not parse connection string: %s",
+ _("out of memory"))));
+
+ /* build a clean connection string from pieces */
+ for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
+ {
+ bool obfuscate;
+
+ /* Skip debug and empty options */
+ if (strchr(conn_opt->dispchar, 'D') ||
+ conn_opt->val == NULL ||
+ conn_opt->val[0] == '\0')
+ continue;
+
+ /* Obfuscate security-sensitive options */
+ obfuscate = strchr(conn_opt->dispchar, '*') != NULL;
+
+ appendPQExpBuffer(&buf, "%s=%s ",
+ conn_opt->keyword,
+ obfuscate ? "********" : conn_opt->val);
+ }
+
+ PQconninfoFree(conn_opts);
+
+ retval = PQExpBufferDataBroken(buf) ? NULL : pstrdup(buf.data);
+ termPQExpBuffer(&buf);
+ return retval;
+}
+
+/*
* Check that primary's system identifier matches ours, and fetch the current
* timeline ID of the primary.
*/
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index ce311cb..04569c2 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -75,6 +75,7 @@ bool hot_standby_feedback;
/* libpqreceiver hooks to these when loaded */
walrcv_connect_type walrcv_connect = NULL;
+walrcv_get_conninfo_type walrcv_get_conninfo = NULL;
walrcv_identify_system_type walrcv_identify_system = NULL;
walrcv_startstreaming_type walrcv_startstreaming = NULL;
walrcv_endstreaming_type walrcv_endstreaming = NULL;
@@ -192,6 +193,7 @@ void
WalReceiverMain(void)
{
char conninfo[MAXCONNINFO];
+ char *tmp_conninfo;
char slotname[NAMEDATALEN];
XLogRecPtr startpoint;
TimeLineID startpointTLI;
@@ -282,7 +284,9 @@ WalReceiverMain(void)
/* Load the libpq-specific functions */
load_file("libpqwalreceiver", false);
- if (walrcv_connect == NULL || walrcv_startstreaming == NULL ||
+ if (walrcv_connect == NULL ||
+ walrcv_get_conninfo == NULL ||
+ walrcv_startstreaming == NULL ||
walrcv_endstreaming == NULL ||
walrcv_identify_system == NULL ||
walrcv_readtimelinehistoryfile == NULL ||
@@ -304,6 +308,20 @@ WalReceiverMain(void)
walrcv_connect(conninfo);
DisableWalRcvImmediateExit();
+ /*
+ * Save user-visible connection string. This clobbers the original
+ * conninfo, for security.
+ */
+ tmp_conninfo = walrcv_get_conninfo();
+ SpinLockAcquire(&walrcv->mutex);
+ memset(walrcv->conninfo, 0, MAXCONNINFO);
+ if (tmp_conninfo)
+ {
+ strlcpy((char *) walrcv->conninfo, tmp_conninfo, MAXCONNINFO);
+ pfree(tmp_conninfo);
+ }
+ SpinLockRelease(&walrcv->mutex);
+
first_stream = true;
for (;;)
{
@@ -1308,10 +1326,9 @@ WalRcvGetStateString(WalRcvState state)
Datum
pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
{
-#define PG_STAT_GET_WAL_RECEIVER_COLS 11
TupleDesc tupdesc;
- Datum values[PG_STAT_GET_WAL_RECEIVER_COLS];
- bool nulls[PG_STAT_GET_WAL_RECEIVER_COLS];
+ Datum *values;
+ bool *nulls;
WalRcvData *walrcv = WalRcv;
WalRcvState state;
XLogRecPtr receive_start_lsn;
@@ -1323,41 +1340,18 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
XLogRecPtr latest_end_lsn;
TimestampTz latest_end_time;
char *slotname;
+ char *conninfo;
/* No WAL receiver, just return a tuple with NULL values */
if (walrcv->pid == 0)
PG_RETURN_NULL();
- /* Initialise values and NULL flags arrays */
- MemSet(values, 0, sizeof(values));
- MemSet(nulls, 0, sizeof(nulls));
+ /* determine result type */
+ if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
+ elog(ERROR, "return type must be a row type");
- /* Initialise attributes information in the tuple descriptor */
- tupdesc = CreateTemplateTupleDesc(PG_STAT_GET_WAL_RECEIVER_COLS, false);
- TupleDescInitEntry(tupdesc, (AttrNumber) 1, "pid",
- INT4OID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 2, "status",
- TEXTOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 3, "receive_start_lsn",
- LSNOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 4, "receive_start_tli",
- INT4OID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 5, "received_lsn",
- LSNOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 6, "received_tli",
- INT4OID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 7, "last_msg_send_time",
- TIMESTAMPTZOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_msg_receipt_time",
- TIMESTAMPTZOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 9, "latest_end_lsn",
- LSNOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 10, "latest_end_time",
- TIMESTAMPTZOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 11, "slot_name",
- TEXTOID, -1, 0);
-
- BlessTupleDesc(tupdesc);
+ values = palloc0(sizeof(Datum) * tupdesc->natts);
+ nulls = palloc0(sizeof(bool) * tupdesc->natts);
/* Take a lock to ensure value consistency */
SpinLockAcquire(&walrcv->mutex);
@@ -1371,6 +1365,7 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
latest_end_lsn = walrcv->latestWalEnd;
latest_end_time = walrcv->latestWalEndTime;
slotname = pstrdup(walrcv->slotname);
+ conninfo = pstrdup(walrcv->conninfo);
SpinLockRelease(&walrcv->mutex);
/* Fetch values */
@@ -1382,7 +1377,7 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
* Only superusers can see details. Other users only get the pid value
* to know whether it is a WAL receiver, but no details.
*/
- MemSet(&nulls[1], true, PG_STAT_GET_WAL_RECEIVER_COLS - 1);
+ MemSet(&nulls[1], true, sizeof(bool) * (tupdesc->natts - 1));
}
else
{
@@ -1418,6 +1413,10 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
nulls[10] = true;
else
values[10] = CStringGetTextDatum(slotname);
+ if (*conninfo == '\0')
+ nulls[11] = true;
+ else
+ values[11] = CStringGetTextDatum(conninfo);
}
/* Returns the record as Datum */
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index f441025..d92c05e 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -2746,7 +2746,7 @@ DATA(insert OID = 3318 ( pg_stat_get_progress_info PGNSP PGUID 12 1 100 0 0
DESCR("statistics: information about progress of backends running maintenance command");
DATA(insert OID = 3099 ( pg_stat_get_wal_senders PGNSP PGUID 12 1 10 0 0 f f f f f t s r 0 0 2249 "" "{23,25,3220,3220,3220,3220,23,25}" "{o,o,o,o,o,o,o,o}" "{pid,state,sent_location,write_location,flush_location,replay_location,sync_priority,sync_state}" _null_ _null_ pg_stat_get_wal_senders _null_ _null_ _null_ ));
DESCR("statistics: information about currently active replication");
-DATA(insert OID = 3317 ( pg_stat_get_wal_receiver PGNSP PGUID 12 1 0 0 0 f f f f f f s r 0 0 2249 "" "{23,25,3220,23,3220,23,1184,1184,3220,1184,25}" "{o,o,o,o,o,o,o,o,o,o,o}" "{pid,status,receive_start_lsn,receive_start_tli,received_lsn,received_tli,last_msg_send_time,last_msg_receipt_time,latest_end_lsn,latest_end_time,slot_name}" _null_ _null_ pg_stat_get_wal_receiver _null_ _null_ _null_ ));
+DATA(insert OID = 3317 ( pg_stat_get_wal_receiver PGNSP PGUID 12 1 0 0 0 f f f f f f s r 0 0 2249 "" "{23,25,3220,23,3220,23,1184,1184,3220,1184,25,25}" "{o,o,o,o,o,o,o,o,o,o,o,o}" "{pid,status,receive_start_lsn,receive_start_tli,received_lsn,received_tli,last_msg_send_time,last_msg_receipt_time,latest_end_lsn,latest_end_time,slot_name,conn_info}" _null_ _null_ pg_stat_get_wal_receiver _null_ _null_ _null_ ));
DESCR("statistics: information about WAL receiver");
DATA(insert OID = 2026 ( pg_backend_pid PGNSP PGUID 12 1 0 0 0 f f f f t f s r 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_backend_pid _null_ _null_ _null_ ));
DESCR("statistics: current backend PID");
diff --git a/src/include/replication/walreceiver.h b/src/include/replication/walreceiver.h
index c87e7a8..cda98ba 100644
--- a/src/include/replication/walreceiver.h
+++ b/src/include/replication/walreceiver.h
@@ -100,7 +100,8 @@ typedef struct
TimestampTz latestWalEndTime;
/*
- * connection string; is used for walreceiver to connect with the primary.
+ * connection string; initially set to connect to the primary, and later
+ * clobbered to hide security-sensitive fields.
*/
char conninfo[MAXCONNINFO];
@@ -133,6 +134,9 @@ extern WalRcvData *WalRcv;
typedef void (*walrcv_connect_type) (char *conninfo);
extern PGDLLIMPORT walrcv_connect_type walrcv_connect;
+typedef char *(*walrcv_get_conninfo_type) (void);
+extern PGDLLIMPORT walrcv_get_conninfo_type walrcv_get_conninfo;
+
typedef void (*walrcv_identify_system_type) (TimeLineID *primary_tli);
extern PGDLLIMPORT walrcv_identify_system_type walrcv_identify_system;
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
--
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 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
wal-receiver-conninfo-v5.patchtext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index bd7bb77..a8b8bb0 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1302,6 +1302,14 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
<entry><type>text</></entry>
<entry>Replication slot name used by this WAL receiver</entry>
</row>
+ <row>
+ <entry><structfield>conn_info</></entry>
+ <entry><type>text</></entry>
+ <entry>
+ Connection string used by this WAL receiver,
+ with security-sensitive fields obfuscated.
+ </entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 272c02f..f52de3a 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -681,7 +681,8 @@ CREATE VIEW pg_stat_wal_receiver AS
s.last_msg_receipt_time,
s.latest_end_lsn,
s.latest_end_time,
- s.slot_name
+ s.slot_name,
+ s.conn_info
FROM pg_stat_get_wal_receiver() s
WHERE s.pid IS NOT NULL;
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index b61e39d..45dccb3 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -20,6 +20,7 @@
#include <sys/time.h>
#include "libpq-fe.h"
+#include "pqexpbuffer.h"
#include "access/xlog.h"
#include "miscadmin.h"
#include "replication/walreceiver.h"
@@ -47,6 +48,7 @@ static char *recvBuf = NULL;
/* Prototypes for interface functions */
static void libpqrcv_connect(char *conninfo);
+static char *libpqrcv_get_conninfo(void);
static void libpqrcv_identify_system(TimeLineID *primary_tli);
static void libpqrcv_readtimelinehistoryfile(TimeLineID tli, char **filename, char **content, int *len);
static bool libpqrcv_startstreaming(TimeLineID tli, XLogRecPtr startpoint,
@@ -74,6 +76,7 @@ _PG_init(void)
walrcv_disconnect != NULL)
elog(ERROR, "libpqwalreceiver already loaded");
walrcv_connect = libpqrcv_connect;
+ walrcv_get_conninfo = libpqrcv_get_conninfo;
walrcv_identify_system = libpqrcv_identify_system;
walrcv_readtimelinehistoryfile = libpqrcv_readtimelinehistoryfile;
walrcv_startstreaming = libpqrcv_startstreaming;
@@ -118,6 +121,55 @@ libpqrcv_connect(char *conninfo)
}
/*
+ * Return a user-displayable conninfo string. Any security-sensitive fields
+ * are obfuscated.
+ */
+static char *
+libpqrcv_get_conninfo(void)
+{
+ PQconninfoOption *conn_opts;
+ PQconninfoOption *conn_opt;
+ PQExpBufferData buf;
+ char *retval;
+
+ Assert(streamConn != NULL);
+
+ initPQExpBuffer(&buf);
+ conn_opts = PQconninfo(streamConn);
+
+ if (conn_opts == NULL)
+ ereport(ERROR,
+ (errmsg("could not parse connection string: %s",
+ _("out of memory"))));
+
+ /* build a clean connection string from pieces */
+ for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
+ {
+ bool obfuscate;
+
+ /* Skip debug and empty options */
+ if (strchr(conn_opt->dispchar, 'D') ||
+ conn_opt->val == NULL ||
+ conn_opt->val[0] == '\0')
+ continue;
+
+ /* Obfuscate security-sensitive options */
+ obfuscate = strchr(conn_opt->dispchar, '*') != NULL;
+
+ appendPQExpBuffer(&buf, "%s%s=%s",
+ buf.len == 0 ? "" : " ",
+ conn_opt->keyword,
+ obfuscate ? "********" : conn_opt->val);
+ }
+
+ PQconninfoFree(conn_opts);
+
+ retval = PQExpBufferDataBroken(buf) ? NULL : pstrdup(buf.data);
+ termPQExpBuffer(&buf);
+ return retval;
+}
+
+/*
* Check that primary's system identifier matches ours, and fetch the current
* timeline ID of the primary.
*/
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index ce311cb..d552f04 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -75,6 +75,7 @@ bool hot_standby_feedback;
/* libpqreceiver hooks to these when loaded */
walrcv_connect_type walrcv_connect = NULL;
+walrcv_get_conninfo_type walrcv_get_conninfo = NULL;
walrcv_identify_system_type walrcv_identify_system = NULL;
walrcv_startstreaming_type walrcv_startstreaming = NULL;
walrcv_endstreaming_type walrcv_endstreaming = NULL;
@@ -192,6 +193,7 @@ void
WalReceiverMain(void)
{
char conninfo[MAXCONNINFO];
+ char *tmp_conninfo;
char slotname[NAMEDATALEN];
XLogRecPtr startpoint;
TimeLineID startpointTLI;
@@ -282,7 +284,9 @@ WalReceiverMain(void)
/* Load the libpq-specific functions */
load_file("libpqwalreceiver", false);
- if (walrcv_connect == NULL || walrcv_startstreaming == NULL ||
+ if (walrcv_connect == NULL ||
+ walrcv_get_conninfo == NULL ||
+ walrcv_startstreaming == NULL ||
walrcv_endstreaming == NULL ||
walrcv_identify_system == NULL ||
walrcv_readtimelinehistoryfile == NULL ||
@@ -304,6 +308,21 @@ WalReceiverMain(void)
walrcv_connect(conninfo);
DisableWalRcvImmediateExit();
+ /*
+ * Save user-visible connection string. This clobbers the original
+ * conninfo, for security.
+ */
+ tmp_conninfo = walrcv_get_conninfo();
+ SpinLockAcquire(&walrcv->mutex);
+ memset(walrcv->conninfo, 0, MAXCONNINFO);
+ if (tmp_conninfo)
+ {
+ strlcpy((char *) walrcv->conninfo, tmp_conninfo, MAXCONNINFO);
+ pfree(tmp_conninfo);
+ }
+ walrcv->ready_to_display = true;
+ SpinLockRelease(&walrcv->mutex);
+
first_stream = true;
for (;;)
{
@@ -1308,10 +1327,9 @@ WalRcvGetStateString(WalRcvState state)
Datum
pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
{
-#define PG_STAT_GET_WAL_RECEIVER_COLS 11
TupleDesc tupdesc;
- Datum values[PG_STAT_GET_WAL_RECEIVER_COLS];
- bool nulls[PG_STAT_GET_WAL_RECEIVER_COLS];
+ Datum *values;
+ bool *nulls;
WalRcvData *walrcv = WalRcv;
WalRcvState state;
XLogRecPtr receive_start_lsn;
@@ -1323,41 +1341,33 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
XLogRecPtr latest_end_lsn;
TimestampTz latest_end_time;
char *slotname;
+ char *conninfo;
/* No WAL receiver, just return a tuple with NULL values */
if (walrcv->pid == 0)
PG_RETURN_NULL();
- /* Initialise values and NULL flags arrays */
- MemSet(values, 0, sizeof(values));
- MemSet(nulls, 0, sizeof(nulls));
+ /*
+ * Users attempting to read this data mustn't be shown security sensitive
+ * data, so sleep until everything has been properly obfuscated.
+ */
+retry:
+ SpinLockAcquire(&walrcv->mutex);
+ if (!walrcv->ready_to_display)
+ {
+ SpinLockRelease(&walrcv->mutex);
+ CHECK_FOR_INTERRUPTS();
+ pg_usleep(1000);
+ goto retry;
+ }
+ SpinLockRelease(&walrcv->mutex);
- /* Initialise attributes information in the tuple descriptor */
- tupdesc = CreateTemplateTupleDesc(PG_STAT_GET_WAL_RECEIVER_COLS, false);
- TupleDescInitEntry(tupdesc, (AttrNumber) 1, "pid",
- INT4OID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 2, "status",
- TEXTOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 3, "receive_start_lsn",
- LSNOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 4, "receive_start_tli",
- INT4OID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 5, "received_lsn",
- LSNOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 6, "received_tli",
- INT4OID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 7, "last_msg_send_time",
- TIMESTAMPTZOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_msg_receipt_time",
- TIMESTAMPTZOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 9, "latest_end_lsn",
- LSNOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 10, "latest_end_time",
- TIMESTAMPTZOID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 11, "slot_name",
- TEXTOID, -1, 0);
+ /* determine result type */
+ if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
+ elog(ERROR, "return type must be a row type");
- BlessTupleDesc(tupdesc);
+ values = palloc0(sizeof(Datum) * tupdesc->natts);
+ nulls = palloc0(sizeof(bool) * tupdesc->natts);
/* Take a lock to ensure value consistency */
SpinLockAcquire(&walrcv->mutex);
@@ -1371,6 +1381,7 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
latest_end_lsn = walrcv->latestWalEnd;
latest_end_time = walrcv->latestWalEndTime;
slotname = pstrdup(walrcv->slotname);
+ conninfo = pstrdup(walrcv->conninfo);
SpinLockRelease(&walrcv->mutex);
/* Fetch values */
@@ -1382,7 +1393,7 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
* Only superusers can see details. Other users only get the pid value
* to know whether it is a WAL receiver, but no details.
*/
- MemSet(&nulls[1], true, PG_STAT_GET_WAL_RECEIVER_COLS - 1);
+ MemSet(&nulls[1], true, sizeof(bool) * (tupdesc->natts - 1));
}
else
{
@@ -1418,6 +1429,10 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
nulls[10] = true;
else
values[10] = CStringGetTextDatum(slotname);
+ if (*conninfo == '\0')
+ nulls[11] = true;
+ else
+ values[11] = CStringGetTextDatum(conninfo);
}
/* Returns the record as Datum */
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index f441025..d92c05e 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -2746,7 +2746,7 @@ DATA(insert OID = 3318 ( pg_stat_get_progress_info PGNSP PGUID 12 1 100 0 0
DESCR("statistics: information about progress of backends running maintenance command");
DATA(insert OID = 3099 ( pg_stat_get_wal_senders PGNSP PGUID 12 1 10 0 0 f f f f f t s r 0 0 2249 "" "{23,25,3220,3220,3220,3220,23,25}" "{o,o,o,o,o,o,o,o}" "{pid,state,sent_location,write_location,flush_location,replay_location,sync_priority,sync_state}" _null_ _null_ pg_stat_get_wal_senders _null_ _null_ _null_ ));
DESCR("statistics: information about currently active replication");
-DATA(insert OID = 3317 ( pg_stat_get_wal_receiver PGNSP PGUID 12 1 0 0 0 f f f f f f s r 0 0 2249 "" "{23,25,3220,23,3220,23,1184,1184,3220,1184,25}" "{o,o,o,o,o,o,o,o,o,o,o}" "{pid,status,receive_start_lsn,receive_start_tli,received_lsn,received_tli,last_msg_send_time,last_msg_receipt_time,latest_end_lsn,latest_end_time,slot_name}" _null_ _null_ pg_stat_get_wal_receiver _null_ _null_ _null_ ));
+DATA(insert OID = 3317 ( pg_stat_get_wal_receiver PGNSP PGUID 12 1 0 0 0 f f f f f f s r 0 0 2249 "" "{23,25,3220,23,3220,23,1184,1184,3220,1184,25,25}" "{o,o,o,o,o,o,o,o,o,o,o,o}" "{pid,status,receive_start_lsn,receive_start_tli,received_lsn,received_tli,last_msg_send_time,last_msg_receipt_time,latest_end_lsn,latest_end_time,slot_name,conn_info}" _null_ _null_ pg_stat_get_wal_receiver _null_ _null_ _null_ ));
DESCR("statistics: information about WAL receiver");
DATA(insert OID = 2026 ( pg_backend_pid PGNSP PGUID 12 1 0 0 0 f f f f t f s r 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_backend_pid _null_ _null_ _null_ ));
DESCR("statistics: current backend PID");
diff --git a/src/include/replication/walreceiver.h b/src/include/replication/walreceiver.h
index c87e7a8..cd787c9 100644
--- a/src/include/replication/walreceiver.h
+++ b/src/include/replication/walreceiver.h
@@ -100,7 +100,8 @@ typedef struct
TimestampTz latestWalEndTime;
/*
- * connection string; is used for walreceiver to connect with the primary.
+ * connection string; initially set to connect to the primary, and later
+ * clobbered to hide security-sensitive fields.
*/
char conninfo[MAXCONNINFO];
@@ -118,6 +119,9 @@ typedef struct
*/
bool force_reply;
+ /* set true once conninfo is ready to display (obfuscated pwds etc) */
+ bool ready_to_display;
+
/*
* Latch used by startup process to wake up walreceiver after telling it
* where to start streaming (after setting receiveStart and
@@ -133,6 +137,9 @@ extern WalRcvData *WalRcv;
typedef void (*walrcv_connect_type) (char *conninfo);
extern PGDLLIMPORT walrcv_connect_type walrcv_connect;
+typedef char *(*walrcv_get_conninfo_type) (void);
+extern PGDLLIMPORT walrcv_get_conninfo_type walrcv_get_conninfo;
+
typedef void (*walrcv_identify_system_type) (TimeLineID *primary_tli);
extern PGDLLIMPORT walrcv_identify_system_type walrcv_identify_system;
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
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 29, 2016 at 11:18 PM, Michael Paquier <michael.paquier@gmail.com
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. 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.
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.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
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 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
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 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
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 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
wal-receiver-fix.patchtext/x-diff; charset=US-ASCII; name=wal-receiver-fix.patchDownload
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index d552f04..9d858ce 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -770,6 +770,7 @@ WalRcvDie(int code, Datum arg)
Assert(walrcv->pid == MyProcPid);
walrcv->walRcvState = WALRCV_STOPPED;
walrcv->pid = 0;
+ walrcv->ready_to_display = false;
SpinLockRelease(&walrcv->mutex);
/* Terminate the connection gracefully. */
@@ -1344,24 +1345,9 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
char *conninfo;
/* No WAL receiver, just return a tuple with NULL values */
- if (walrcv->pid == 0)
+ if (walrcv->pid == 0 || !walrcv->ready_to_display)
PG_RETURN_NULL();
- /*
- * Users attempting to read this data mustn't be shown security sensitive
- * data, so sleep until everything has been properly obfuscated.
- */
-retry:
- SpinLockAcquire(&walrcv->mutex);
- if (!walrcv->ready_to_display)
- {
- SpinLockRelease(&walrcv->mutex);
- CHECK_FOR_INTERRUPTS();
- pg_usleep(1000);
- goto retry;
- }
- SpinLockRelease(&walrcv->mutex);
-
/* determine result type */
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
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, 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
--
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, 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
Attachments:
wal-receiver-conninfo.patchtext/x-diff; charset=US-ASCII; name=wal-receiver-conninfo.patchDownload
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index a8b8bb0..b620feb 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1303,7 +1303,7 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
<entry>Replication slot name used by this WAL receiver</entry>
</row>
<row>
- <entry><structfield>conn_info</></entry>
+ <entry><structfield>conninfo</></entry>
<entry><type>text</></entry>
<entry>
Connection string used by this WAL receiver,
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index f52de3a..4fc5d5a 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -682,7 +682,7 @@ CREATE VIEW pg_stat_wal_receiver AS
s.latest_end_lsn,
s.latest_end_time,
s.slot_name,
- s.conn_info
+ s.conninfo
FROM pg_stat_get_wal_receiver() s
WHERE s.pid IS NOT NULL;
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index d92c05e..5d233e3 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -2746,7 +2746,7 @@ DATA(insert OID = 3318 ( pg_stat_get_progress_info PGNSP PGUID 12 1 100 0 0
DESCR("statistics: information about progress of backends running maintenance command");
DATA(insert OID = 3099 ( pg_stat_get_wal_senders PGNSP PGUID 12 1 10 0 0 f f f f f t s r 0 0 2249 "" "{23,25,3220,3220,3220,3220,23,25}" "{o,o,o,o,o,o,o,o}" "{pid,state,sent_location,write_location,flush_location,replay_location,sync_priority,sync_state}" _null_ _null_ pg_stat_get_wal_senders _null_ _null_ _null_ ));
DESCR("statistics: information about currently active replication");
-DATA(insert OID = 3317 ( pg_stat_get_wal_receiver PGNSP PGUID 12 1 0 0 0 f f f f f f s r 0 0 2249 "" "{23,25,3220,23,3220,23,1184,1184,3220,1184,25,25}" "{o,o,o,o,o,o,o,o,o,o,o,o}" "{pid,status,receive_start_lsn,receive_start_tli,received_lsn,received_tli,last_msg_send_time,last_msg_receipt_time,latest_end_lsn,latest_end_time,slot_name,conn_info}" _null_ _null_ pg_stat_get_wal_receiver _null_ _null_ _null_ ));
+DATA(insert OID = 3317 ( pg_stat_get_wal_receiver PGNSP PGUID 12 1 0 0 0 f f f f f f s r 0 0 2249 "" "{23,25,3220,23,3220,23,1184,1184,3220,1184,25,25}" "{o,o,o,o,o,o,o,o,o,o,o,o}" "{pid,status,receive_start_lsn,receive_start_tli,received_lsn,received_tli,last_msg_send_time,last_msg_receipt_time,latest_end_lsn,latest_end_time,slot_name,conninfo}" _null_ _null_ pg_stat_get_wal_receiver _null_ _null_ _null_ ));
DESCR("statistics: information about WAL receiver");
DATA(insert OID = 2026 ( pg_backend_pid PGNSP PGUID 12 1 0 0 0 f f f f t f s r 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_backend_pid _null_ _null_ _null_ ));
DESCR("statistics: current backend PID");
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 125c31b..ad44ae2 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1893,8 +1893,8 @@ pg_stat_wal_receiver| SELECT s.pid,
s.latest_end_lsn,
s.latest_end_time,
s.slot_name,
- s.conn_info
- FROM pg_stat_get_wal_receiver() s(pid, status, receive_start_lsn, receive_start_tli, received_lsn, received_tli, last_msg_send_time, last_msg_receipt_time, latest_end_lsn, latest_end_time, slot_name, conn_info)
+ s.conninfo
+ FROM pg_stat_get_wal_receiver() s(pid, status, receive_start_lsn, receive_start_tli, received_lsn, received_tli, last_msg_send_time, last_msg_receipt_time, latest_end_lsn, latest_end_time, slot_name, conninfo)
WHERE (s.pid IS NOT NULL);
pg_stat_xact_all_tables| SELECT c.oid AS relid,
n.nspname AS schemaname,
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers