Обсуждение: Allow pg_read_all_stats to read pg_stat_progress_*
Hi!
One of our users asked me why they cannot read details of pg_stat_progress_vacuum while they have pg_read_all_stats
role.
Maybe I'm missing something, but I think they should be able to read stats...
PFA fix.
This affects pg_stat_progress_analyze, pg_stat_progress_basebackup, pg_stat_progress_cluster,
pg_stat_progress_create_indexand pg_stat_progress_vacuum.
With patch
postgres=# set role pg_read_all_stats ;
postgres=> select * from pg_stat_progress_vacuum ;
pid | datid | datname | relid | phase | heap_blks_total | heap_blks_scanned | heap_blks_vacuumed |
index_vacuum_count| max_dead_tuples | num_dead_tuples
-------+-------+----------+-------+---------------+-----------------+-------------------+--------------------+--------------------+-----------------+-----------------
76331 | 12923 | postgres | 1247 | scanning heap | 10 | 1 | 0 |
0 | 2910 | 0
(1 row)
Without patch
postgres=# set role pg_read_all_stats ;
SET
postgres=> select * from pg_stat_progress_vacuum ;
pid | datid | datname | relid | phase | heap_blks_total | heap_blks_scanned | heap_blks_vacuumed |
index_vacuum_count| max_dead_tuples | num_dead_tuples
-------+-------+----------+-------+-------+-----------------+-------------------+--------------------+--------------------+-----------------+-----------------
76331 | 12923 | postgres | | | | | |
| |
(1 row)
Thanks!
Best regards, Andrey Borodin.
Вложения
On Wed, Apr 15, 2020 at 9:14 AM Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
Hi!
One of our users asked me why they cannot read details of pg_stat_progress_vacuum while they have pg_read_all_stats role.
Maybe I'm missing something, but I think they should be able to read stats...
PFA fix.
This affects pg_stat_progress_analyze, pg_stat_progress_basebackup, pg_stat_progress_cluster, pg_stat_progress_create_index and pg_stat_progress_vacuum.
With patch
postgres=# set role pg_read_all_stats ;
postgres=> select * from pg_stat_progress_vacuum ;
pid | datid | datname | relid | phase | heap_blks_total | heap_blks_scanned | heap_blks_vacuumed | index_vacuum_count | max_dead_tuples | num_dead_tuples
-------+-------+----------+-------+---------------+-----------------+-------------------+--------------------+--------------------+-----------------+-----------------
76331 | 12923 | postgres | 1247 | scanning heap | 10 | 1 | 0 | 0 | 2910 | 0
(1 row)
Without patch
postgres=# set role pg_read_all_stats ;
SET
postgres=> select * from pg_stat_progress_vacuum ;
pid | datid | datname | relid | phase | heap_blks_total | heap_blks_scanned | heap_blks_vacuumed | index_vacuum_count | max_dead_tuples | num_dead_tuples
-------+-------+----------+-------+-------+-----------------+-------------------+--------------------+--------------------+-----------------+-----------------
76331 | 12923 | postgres | | | | | | | |
(1 row)
I think that makes perfect sense. The documentation explicitly says "can read all pg_stat_* views", which is clearly wrong -- so either the code or the docs should be fixed, and it looks like it's the code that should be fixed to me.
As for the patch, one could argue that we should just store the resulting boolean instead of re-running the check (e.g. have a "bool has_stats_privilege" or such), but perhaps that's an unnecessary micro-optimization, like the attached.
Вложения
> 15 апр. 2020 г., в 15:25, Magnus Hagander <magnus@hagander.net> написал(а): > > > I think that makes perfect sense. The documentation explicitly says "can read all pg_stat_* views", which is clearly wrong-- so either the code or the docs should be fixed, and it looks like it's the code that should be fixed to me. Should it be bug or v14 feature? Also pgstatfuncs.c contains a lot more checks of has_privs_of_role(GetUserId(), beentry->st_userid). Maybe grant them all? > As for the patch, one could argue that we should just store the resulting boolean instead of re-running the check (e.g.have a "bool has_stats_privilege" or such), but perhaps that's an unnecessary micro-optimization, like the attached. Looks good to me. Thanks! Best regards, Andrey Borodin.
At Wed, 15 Apr 2020 15:58:05 +0500, "Andrey M. Borodin" <x4mmm@yandex-team.ru> wrote in > > 15 апр. 2020 г., в 15:25, Magnus Hagander <magnus@hagander.net> написал(а): > > I think that makes perfect sense. The documentation explicitly says "can read all pg_stat_* views", which is clearlywrong -- so either the code or the docs should be fixed, and it looks like it's the code that should be fixed to me. > Should it be bug or v14 feature? > > Also pgstatfuncs.c contains a lot more checks of has_privs_of_role(GetUserId(), beentry->st_userid). > Maybe grant them all? > > > As for the patch, one could argue that we should just store the resulting boolean instead of re-running the check (e.g.have a "bool has_stats_privilege" or such), but perhaps that's an unnecessary micro-optimization, like the attached. > > Looks good to me. pg_stat_get_activty checks (has_privs_of_role() || is_member_of_role()) in-place for every entry. It's not necessary but I suppose that doing the same thing for pg_stat_progress_info might be better. It's another issue, but pg_stat_get_backend_* functions don't consider pg_read_all_stats. I suppose that the functions should work under the same criteria to pg_stat views, and maybe explicitly documented? If we do that, it may be better that we define "PGSTAT_VIEW_PRIV()" or something like and replace the all occurances of the idiomatic condition with it. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Thu, Apr 16, 2020 at 7:05 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
At Wed, 15 Apr 2020 15:58:05 +0500, "Andrey M. Borodin" <x4mmm@yandex-team.ru> wrote in
> > 15 апр. 2020 г., в 15:25, Magnus Hagander <magnus@hagander.net> написал(а):
> > I think that makes perfect sense. The documentation explicitly says "can read all pg_stat_* views", which is clearly wrong -- so either the code or the docs should be fixed, and it looks like it's the code that should be fixed to me.
> Should it be bug or v14 feature?
>
> Also pgstatfuncs.c contains a lot more checks of has_privs_of_role(GetUserId(), beentry->st_userid).
> Maybe grant them all?
>
> > As for the patch, one could argue that we should just store the resulting boolean instead of re-running the check (e.g. have a "bool has_stats_privilege" or such), but perhaps that's an unnecessary micro-optimization, like the attached.
>
> Looks good to me.
pg_stat_get_activty checks (has_privs_of_role() ||
is_member_of_role()) in-place for every entry. It's not necessary but
I suppose that doing the same thing for pg_stat_progress_info might be
better.
From a result perspective, it shouldn't make a difference though, should it? It's a micro-optimization, but it might not have an actual performance effect in reality as well, but the result should always be the same?
(FWIW, pg_stat_statements has a coding pattern similar to the one I suggested in the patch)
It's another issue, but pg_stat_get_backend_* functions don't consider
pg_read_all_stats. I suppose that the functions should work under the
same criteria to pg_stat views, and maybe explicitly documented?
That's a good question. They haven't been documented to do so, but it certainly seems *weird* that the same information should be available through a view like pg_stat_activity, but not through the functions.
I would guess this was simply forgotten in 25fff40798f -- I don't recall any discussion about it. The commit message specifically says pg_database_size() and pg_tablespace_size(), but mentions nothing about pg_stat_*.
If we do that, it may be better that we define "PGSTAT_VIEW_PRIV()" or
something like and replace the all occurances of the idiomatic
condition with it.
You mean something like the attached?
Вложения
At Thu, 16 Apr 2020 14:46:00 +0200, Magnus Hagander <magnus@hagander.net> wrote in > On Thu, Apr 16, 2020 at 7:05 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> > wrote: > > > At Wed, 15 Apr 2020 15:58:05 +0500, "Andrey M. Borodin" < > > x4mmm@yandex-team.ru> wrote in > > > > 15 апр. 2020 г., в 15:25, Magnus Hagander <magnus@hagander.net> > > написал(а): > > > > I think that makes perfect sense. The documentation explicitly says > > "can read all pg_stat_* views", which is clearly wrong -- so either the > > code or the docs should be fixed, and it looks like it's the code that > > should be fixed to me. > > > Should it be bug or v14 feature? > > > > > > Also pgstatfuncs.c contains a lot more checks of > > has_privs_of_role(GetUserId(), beentry->st_userid). > > > Maybe grant them all? > > > > > > > As for the patch, one could argue that we should just store the > > resulting boolean instead of re-running the check (e.g. have a "bool > > has_stats_privilege" or such), but perhaps that's an unnecessary > > micro-optimization, like the attached. > > > > > > Looks good to me. > > > > pg_stat_get_activty checks (has_privs_of_role() || > > is_member_of_role()) in-place for every entry. It's not necessary but > > I suppose that doing the same thing for pg_stat_progress_info might be > > better. > > > > From a result perspective, it shouldn't make a difference though, should > it? It's a micro-optimization, but it might not have an actual performance > effect in reality as well, but the result should always be the same? > > (FWIW, pg_stat_statements has a coding pattern similar to the one I > suggested in the patch) As a priciple, I prefer the "optimized" (or pg_stat_statements') pattern because that style suggests that the privilege is (shold be) same to all entries, not because that it might be a bit faster. My suggestion above is just from "same style with a nearby code". But at least the v2 code introduces the third style (mixture of in-place and pre-evaluated) seemed a kind of ad-hoc. > > It's another issue, but pg_stat_get_backend_* functions don't consider > > pg_read_all_stats. I suppose that the functions should work under the > > same criteria to pg_stat views, and maybe explicitly documented? > > > > That's a good question. They haven't been documented to do so, but it > certainly seems *weird* that the same information should be available > through a view like pg_stat_activity, but not through the functions. > > I would guess this was simply forgotten in 25fff40798f -- I don't recall > any discussion about it. The commit message specifically says > pg_database_size() and pg_tablespace_size(), but mentions nothing about > pg_stat_*. Yeah. pg_database_size() ERRORs out for insufficient privileges. On the other hand pg_stat_* returns "<insufficient privilege>" not ERRORing out. For example, pg_stat_get_backend_wait_event_type is documented as https://www.postgresql.org/docs/current/monitoring-stats.html#MONITORING-STATS-BACKEND-FUNCS-TABLE "Wait event type name if backend is currently waiting, otherwise NULL. See Table 27.4 for details." I would read this as "If the function returns non-null value, the returned value represents the wait event type mentioned in Table 27.4", but, "<insufficient privilege>" is not a wait event type. I think something like "text-returning functions may return some out-of-the-domain strings like "<insufficient privilege>" under corresponding conditions". > > If we do that, it may be better that we define "PGSTAT_VIEW_PRIV()" or > > something like and replace the all occurances of the idiomatic > > condition with it. > > > > You mean something like the attached? Exactly. It looks good to me. Thanks! regards. -- Kyotaro Horiguchi NTT Open Source Software Center
> 16 апр. 2020 г., в 17:46, Magnus Hagander <magnus@hagander.net> написал(а): > > > If we do that, it may be better that we define "PGSTAT_VIEW_PRIV()" or > something like and replace the all occurances of the idiomatic > condition with it. > > You mean something like the attached? > > <allow_read_all_stats3.diff> Is it correct that we use DEFAULT_ROLE_READ_ALL_STATS regardless of inheritance? I'm not familiar with what is inheritedand what is not, so I think it's better to ask explicitly. +#define HAS_PGSTAT_PERMISSIONS(role) (is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_STATS) || has_privs_of_role(GetUserId(),role)) Besides this, the patch looks good to me. Thanks! Best regards, Andrey Borodin,
On Mon, Apr 20, 2020 at 12:43 PM Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
> 16 апр. 2020 г., в 17:46, Magnus Hagander <magnus@hagander.net> написал(а):
>
>
> If we do that, it may be better that we define "PGSTAT_VIEW_PRIV()" or
> something like and replace the all occurances of the idiomatic
> condition with it.
>
> You mean something like the attached?
>
> <allow_read_all_stats3.diff>
Is it correct that we use DEFAULT_ROLE_READ_ALL_STATS regardless of inheritance? I'm not familiar with what is inherited and what is not, so I think it's better to ask explicitly.
+#define HAS_PGSTAT_PERMISSIONS(role) (is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_STATS) || has_privs_of_role(GetUserId(), role))
It is consistent with all the other uses of DEFAULT_ROLE_READ_ALL_STATS that I can find.
Besides this, the patch looks good to me.
Thanks, I've pushed it now.
Greetings, * Magnus Hagander (magnus@hagander.net) wrote: > On Mon, Apr 20, 2020 at 12:43 PM Andrey M. Borodin <x4mmm@yandex-team.ru> > wrote: > > > 16 апр. 2020 г., в 17:46, Magnus Hagander <magnus@hagander.net> > > написал(а): > > > If we do that, it may be better that we define "PGSTAT_VIEW_PRIV()" or > > > something like and replace the all occurances of the idiomatic > > > condition with it. > > > > > > You mean something like the attached? > > > > > > <allow_read_all_stats3.diff> > > > > Is it correct that we use DEFAULT_ROLE_READ_ALL_STATS regardless of > > inheritance? I'm not familiar with what is inherited and what is not, so I > > think it's better to ask explicitly. > > > > +#define HAS_PGSTAT_PERMISSIONS(role) (is_member_of_role(GetUserId(), > > DEFAULT_ROLE_READ_ALL_STATS) || has_privs_of_role(GetUserId(), role)) > > It is consistent with all the other uses of DEFAULT_ROLE_READ_ALL_STATS > that I can find. Ugh. That doesn't make it correct though.. We really should be using has_privs_of_role() for these cases (and that goes for all of the default role cases- some of which are correct and others are not, it seems). Thanks, Stephen
Вложения
Stephen Frost <sfrost@snowman.net> writes:
> Ugh. That doesn't make it correct though.. We really should be using
> has_privs_of_role() for these cases (and that goes for all of the
> default role cases- some of which are correct and others are not, it
> seems).
I have a different concern about this patch: while reading statistical
values is fine, do we REALLY want pg_read_all_stats to enable
pg_stat_get_activity(), ie viewing other sessions' command strings?
That opens security considerations that don't seem to me to be covered
by the description of the role.
regards, tom lane
On Mon, Apr 20, 2020 at 4:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Stephen Frost <sfrost@snowman.net> writes:
> Ugh. That doesn't make it correct though.. We really should be using
> has_privs_of_role() for these cases (and that goes for all of the
> default role cases- some of which are correct and others are not, it
> seems).
I have a different concern about this patch: while reading statistical
values is fine, do we REALLY want pg_read_all_stats to enable
pg_stat_get_activity(), ie viewing other sessions' command strings?
That opens security considerations that don't seem to me to be covered
by the description of the role.
It already did allow that, and that's fully documented.
The patch only adds the ability to get at it through functions, but not through views. (And the pg_stat_progress_* views).
The pg_stat_activity change is only:
nulls[16] = true;
/* Values only available to role member or pg_read_all_stats */
- if (has_privs_of_role(GetUserId(), beentry->st_userid) ||
- is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_STATS))
+ if (HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
{
SockAddr zero_clientaddr;
char *clipped_activity;
Which moves the check into the macro, but doesn't change how it works.