Provide list of subscriptions and publications in psql's completion

On Thu, Feb 2, 2017 at 2:48 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Hi all,

While testing a bit the logical replication facility, I have bumped on
the fast that psql's completion does not show the list of things
already created. Attached is a patch.

+#define Query_for_list_of_subscriptions \
+" SELECT pg_catalog.quote_ident(subname) "\
+"   FROM pg_catalog.pg_subscription "\
+"  WHERE substring(pg_catalog.quote_ident(subname),1,%d)='%s'"

Since non-superuser is not allowed to access to pg_subscription,
pg_stat_subscription should be accessed here, instead. Thought?

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 2/2/17 12:48 PM, Fujii Masao wrote:

+#define Query_for_list_of_subscriptions \
+" SELECT pg_catalog.quote_ident(subname) "\
+"   FROM pg_catalog.pg_subscription "\
+"  WHERE substring(pg_catalog.quote_ident(subname),1,%d)='%s'"

Since non-superuser is not allowed to access to pg_subscription,
pg_stat_subscription should be accessed here, instead. Thought?

Arguably, you could leave it like that, assuming it fails cleanly for
nonsuperusers. Nonsuperusers are not going to be able to run any
commands on subscriptions anyway, so there is little use for it.

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

On 2/2/17 12:48 AM, Michael Paquier wrote:

+#define Query_for_list_of_subscriptions \
+" SELECT pg_catalog.quote_ident(subname) "\
+"   FROM pg_catalog.pg_subscription "\
+"  WHERE substring(pg_catalog.quote_ident(subname),1,%d)='%s'"

This query should also be qualified by current database.

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

On Fri, Feb 3, 2017 at 3:56 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 2/2/17 12:48 AM, Michael Paquier wrote:

+#define Query_for_list_of_subscriptions \
+" SELECT pg_catalog.quote_ident(subname) "\
+"   FROM pg_catalog.pg_subscription "\
+"  WHERE substring(pg_catalog.quote_ident(subname),1,%d)='%s'"

This query should also be qualified by current database.

Indeed. Here is an updated patch.
--
Michael

On Fri, Feb 3, 2017 at 3:55 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 2/2/17 12:48 PM, Fujii Masao wrote:

+#define Query_for_list_of_subscriptions \
+" SELECT pg_catalog.quote_ident(subname) "\
+"   FROM pg_catalog.pg_subscription "\
+"  WHERE substring(pg_catalog.quote_ident(subname),1,%d)='%s'"

Since non-superuser is not allowed to access to pg_subscription,
pg_stat_subscription should be accessed here, instead. Thought?

Arguably, you could leave it like that, assuming it fails cleanly for
nonsuperusers. Nonsuperusers are not going to be able to run any
commands on subscriptions anyway, so there is little use for it.

No. You can get rid of superuser privilege from the owner of the subscription
and that nonsuperuser owner can run some commands on the subscriptin.
It's a bit artificial, but you can. I'm not sure if we should add the check to
prevent the owner from becoming nonsuperuser. But if the owner always must
have a superuser privilege per the specification of logical replication,
I think that such check would be necessary.

Also I prefer to tab-complete the subscriptions even for nonsuperusers.
There are some objects that only superuser or owner can manage, but their
names are currently tab-completed even when current user is "normal" one.
So I'm afraid that handling only subscriptions differently might be more
confusing.

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 Sat, Feb 4, 2017 at 9:01 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Fri, Feb 3, 2017 at 3:56 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 2/2/17 12:48 AM, Michael Paquier wrote:

+#define Query_for_list_of_subscriptions \
+" SELECT pg_catalog.quote_ident(subname) "\
+"   FROM pg_catalog.pg_subscription "\
+"  WHERE substring(pg_catalog.quote_ident(subname),1,%d)='%s'"

This query should also be qualified by current database.

Indeed. Here is an updated patch.

With this patch, when normal users type TAB after SUBSCRIPTION,
they got "permission denied" error. I don't think that's acceptable.

In "CREATE SUBSCRIPTION ... PUBLICATION" and "ALTER SUBSCRIPTION ... SET
PUBLICATION" cases, the publication defined in the publisher side should be
specified. But, with this patch, the tab-completion for PUBLICATION shows
the publications defined in local server (ie, subscriber side in those cases).
This might be confusing.

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 Sun, Feb 5, 2017 at 5:04 AM, Fujii Masao <masao.fujii@gmail.com> wrote:

With this patch, when normal users type TAB after SUBSCRIPTION,
they got "permission denied" error. I don't think that's acceptable.

Right, I can see that now. pg_stat_get_subscription() does not offer
as well a way to filter by databases... Perhaps we could add it? it is
stored as LogicalRepWorker->dbid so making it visible is sort of easy.

In "CREATE SUBSCRIPTION ... PUBLICATION" and "ALTER SUBSCRIPTION ... SET
PUBLICATION" cases, the publication defined in the publisher side should be
specified. But, with this patch, the tab-completion for PUBLICATION shows
the publications defined in local server (ie, subscriber side in those cases).
This might be confusing.

Which would mean that psql tab completion should try to connect the
remote server using the connection string defined with the
subscription to get this information, which looks unsafe to me. To be
honest, I'd rather see a list of local objects defined than nothing..
--
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, Feb 6, 2017 at 1:52 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Sun, Feb 5, 2017 at 5:04 AM, Fujii Masao <masao.fujii@gmail.com> wrote:

With this patch, when normal users type TAB after SUBSCRIPTION,
they got "permission denied" error. I don't think that's acceptable.

Right, I can see that now. pg_stat_get_subscription() does not offer
as well a way to filter by databases... Perhaps we could add it? it is
stored as LogicalRepWorker->dbid so making it visible is sort of easy.

Yes, that's an option. And, if we add dbid to pg_stat_subscription,
I'm tempted to add all the pg_subscription's columns except subconninfo
into pg_stat_subscription. Since subconninfo may contain security-sensitive
information, it should be excluded. But it seems useful to expose other
columns. Then, if we expose all the columns except subconninfo, maybe
it's better to just revoke subconninfo column on pg_subscription instead of
all columns. Thought?

BTW, I found that psql's \dRs command has the same problem;
the permission denied error occurs when normal user runs \dRs.
This issue should be fixed in the same way as tab-completion one.

Also I found that tab-completion for psql's meta-commands doesn't
show \dRp and \dRs.

In "CREATE SUBSCRIPTION ... PUBLICATION" and "ALTER SUBSCRIPTION ... SET
PUBLICATION" cases, the publication defined in the publisher side should be
specified. But, with this patch, the tab-completion for PUBLICATION shows
the publications defined in local server (ie, subscriber side in those cases).
This might be confusing.

Which would mean that psql tab completion should try to connect the
remote server using the connection string defined with the
subscription to get this information, which looks unsafe to me. To be
honest, I'd rather see a list of local objects defined than nothing..

IMO showing nothing is better. But many people think that even local
objects should be showed in that case, I can live with that.

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 2/5/17 11:52 PM, Michael Paquier wrote:

In "CREATE SUBSCRIPTION ... PUBLICATION" and "ALTER SUBSCRIPTION ... SET
PUBLICATION" cases, the publication defined in the publisher side should be
specified. But, with this patch, the tab-completion for PUBLICATION shows
the publications defined in local server (ie, subscriber side in those cases).
This might be confusing.

Which would mean that psql tab completion should try to connect the
remote server using the connection string defined with the
subscription to get this information, which looks unsafe to me. To be
honest, I'd rather see a list of local objects defined than nothing..

But it's the wrong list.

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

On 2/6/17 10:54 AM, Fujii Masao wrote:

Yes, that's an option. And, if we add dbid to pg_stat_subscription,
I'm tempted to add all the pg_subscription's columns except subconninfo
into pg_stat_subscription. Since subconninfo may contain security-sensitive
information, it should be excluded. But it seems useful to expose other
columns. Then, if we expose all the columns except subconninfo, maybe
it's better to just revoke subconninfo column on pg_subscription instead of
all columns. Thought?

I think previous practice is to provide a view such as pg_subscriptions
that contains all the nonprivileged information.

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

On Tue, Feb 7, 2017 at 6:35 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 2/6/17 10:54 AM, Fujii Masao wrote:

Yes, that's an option. And, if we add dbid to pg_stat_subscription,
I'm tempted to add all the pg_subscription's columns except subconninfo
into pg_stat_subscription. Since subconninfo may contain security-sensitive
information, it should be excluded. But it seems useful to expose other
columns. Then, if we expose all the columns except subconninfo, maybe
it's better to just revoke subconninfo column on pg_subscription instead of
all columns. Thought?

I think previous practice is to provide a view such as pg_subscriptions
that contains all the nonprivileged information.

OK, I think that I see the point you are coming at:
pg_stat_get_subscription (or stat tables) should not be used for
psql's tab completion. So gathering all things discussed, we have:
- No tab completion for publications.
- Fix the meta-commands.
- Addition of a view pg_subscriptions with all the non-sensitive data.
(- Or really extend pg_stat_subscriptions with the database ID and use
it for tab completion?)
Am I missing something?
--
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, Feb 10, 2017 at 1:52 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Tue, Feb 7, 2017 at 6:35 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 2/6/17 10:54 AM, Fujii Masao wrote:

Yes, that's an option. And, if we add dbid to pg_stat_subscription,
I'm tempted to add all the pg_subscription's columns except subconninfo
into pg_stat_subscription. Since subconninfo may contain security-sensitive
information, it should be excluded. But it seems useful to expose other
columns. Then, if we expose all the columns except subconninfo, maybe
it's better to just revoke subconninfo column on pg_subscription instead of
all columns. Thought?

I think previous practice is to provide a view such as pg_subscriptions
that contains all the nonprivileged information.

OK, I think that I see the point you are coming at:
pg_stat_get_subscription (or stat tables) should not be used for
psql's tab completion. So gathering all things discussed, we have:
- No tab completion for publications.

No, the tab-completions for ALTER/DROP PUBLICATION should show the local
publications because those commands drop and alter the local ones. OTOH,
CREATE/ALTER SUBSCRIPTOIN ... PUBLICATION should show nothing because
the remote publications in the publisher side should be specified there.

- Fix the meta-commands.

Yes.

- Addition of a view pg_subscriptions with all the non-sensitive data.
(- Or really extend pg_stat_subscriptions with the database ID and use
it for tab completion?)

Probably I failed to get Peter's point... Anyway IMO that we can expose all the
columns except the sensitive information (i.e., subconninfo field)
in pg_subscription to even non-superusers. Then we can use pg_subscription
for the tab-completion for ALTER/DROP SUBSCRIPTION.

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 Tue, Feb 14, 2017 at 2:07 AM, Fujii Masao <masao.fujii@gmail.com> wrote:

No, the tab-completions for ALTER/DROP PUBLICATION should show the local
publications because those commands drop and alter the local ones. OTOH,
CREATE/ALTER SUBSCRIPTOIN ... PUBLICATION should show nothing because
the remote publications in the publisher side should be specified there.

Doh, yes. You are right about that.

- Addition of a view pg_subscriptions with all the non-sensitive data.
(- Or really extend pg_stat_subscriptions with the database ID and use
it for tab completion?)

Probably I failed to get Peter's point... Anyway IMO that we can expose all the
columns except the sensitive information (i.e., subconninfo field)
in pg_subscription to even non-superusers. Then we can use pg_subscription
for the tab-completion for ALTER/DROP SUBSCRIPTION.

To be honest, I find subconninfo quite useful to check where a
subscription is getting its changes from, so I'd rather not touch it.
It looks as well a bit overkill to just create a new view on an object
type non-superusers cannot even use... There are already 1 view and 1
system catalog related to it, so I'd be of the opinion to let it fail
silently with a permission error and keep it as an empty list for
them.
--
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 2/13/17 9:36 PM, Michael Paquier wrote:

Probably I failed to get Peter's point... Anyway IMO that we can expose all the
columns except the sensitive information (i.e., subconninfo field)
in pg_subscription to even non-superusers. Then we can use pg_subscription
for the tab-completion for ALTER/DROP SUBSCRIPTION.

To be honest, I find subconninfo quite useful to check where a
subscription is getting its changes from, so I'd rather not touch it.
It looks as well a bit overkill to just create a new view on an object
type non-superusers cannot even use... There are already 1 view and 1
system catalog related to it, so I'd be of the opinion to let it fail
silently with a permission error and keep it as an empty list for
them.

Why not do what we do for pg_stat_activity.current_query and leave it
NULL for non-SU?

Even better would be if we could simply strip out password info.
Presumably we already know how to parse the contents, so I'd think that
shouldn't be that difficult.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)

--
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, Feb 15, 2017 at 3:18 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:

Why not do what we do for pg_stat_activity.current_query and leave it NULL for non-SU?

If subcriptions are designed to be superuser-only, it seems fair to me
to do so. Some other system SRFs do that already.

Even better would be if we could simply strip out password info. Presumably
we already know how to parse the contents, so I'd think that shouldn't be
that difficult.

I thought that this was correctly clobbered... But... No that's not
the case by looking at the code. And honestly I think that it is
unacceptable to show potentially security-sensitive information in
system catalogs via a connection string. We are really careful about
not showing anything bad in pg_stat_wal_receiver, which also sets to
NULL fields for non-superusers and even clobbered values in the
printed connection string for superusers, but pg_subscription fails on
those points.

I am adding an open item on the wiki regarding that. FWIW, a patch
needs to refactor libpqrcv_check_conninfo and libpqrcv_get_conninfo so
as the connection string build of PQconninfoOption data goes through
the same process. If everybody agrees on those lines, I have no
problems in producing 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

On 2/13/17 12:07, Fujii Masao wrote:

Anyway IMO that we can expose all the
columns except the sensitive information (i.e., subconninfo field)
in pg_subscription to even non-superusers.

You mean with column privileges?

We could probably do that. I don't know if we have done that before on
system catalogs.

--
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) wrote:

On 2/13/17 12:07, Fujii Masao wrote:

Anyway IMO that we can expose all the
columns except the sensitive information (i.e., subconninfo field)
in pg_subscription to even non-superusers.

You mean with column privileges?

We could probably do that. I don't know if we have done that before on
system catalogs.

I don't believe we have, though I'm not against doing so. I can't think
of any reason off-hand why it wouldn't work.

If we did that then perhaps we could remove some of the other views that
we have which are just to provide a subset of the columns of some other
table (eg: pg_roles)...

Thanks!

Stephen

On 15/02/17 05:56, Michael Paquier wrote:

On Wed, Feb 15, 2017 at 3:18 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:

Why not do what we do for pg_stat_activity.current_query and leave it NULL for non-SU?

If subcriptions are designed to be superuser-only, it seems fair to me
to do so. Some other system SRFs do that already.

Even better would be if we could simply strip out password info. Presumably
we already know how to parse the contents, so I'd think that shouldn't be
that difficult.

I thought that this was correctly clobbered... But... No that's not
the case by looking at the code. And honestly I think that it is
unacceptable to show potentially security-sensitive information in
system catalogs via a connection string. We are really careful about
not showing anything bad in pg_stat_wal_receiver, which also sets to
NULL fields for non-superusers and even clobbered values in the
printed connection string for superusers, but pg_subscription fails on
those points.

I am not following here, pg_subscription is currently superuser only
catalog, similarly to pg_user_mapping, there is no leaking.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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, Feb 18, 2017 at 11:57 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:

On 15/02/17 05:56, Michael Paquier wrote:

I thought that this was correctly clobbered... But... No that's not
the case by looking at the code. And honestly I think that it is
unacceptable to show potentially security-sensitive information in
system catalogs via a connection string. We are really careful about
not showing anything bad in pg_stat_wal_receiver, which also sets to
NULL fields for non-superusers and even clobbered values in the
printed connection string for superusers, but pg_subscription fails on
those points.

I am not following here, pg_subscription is currently superuser only
catalog, similarly to pg_user_mapping, there is no leaking.

Even if it is a superuser-only view, pg_subscription does not hide
sensitive values in connection strings while it should. See similar
discussion for pg_stat_wal_receiver here which is also superuser-only
(it does display null values for non-superusers):
/messages/by-id/562f6c7f-6a47-0a8a-e189-2de9ea896849@2ndquadrant.com
Something needs to be done at least for that, independently on the
psql completion.
--
Michael

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