Revoke execution permission of pg_stat_statements_reset() from pg_read_all_stats role

Started by Haribabu Kommiover 7 years ago5 messages

kommi.haribabu@gmail.com

over 7 years ago

1 attachment(s)

[spawning a new thread]

In commit 25fff40798 the execute permission of pg_stat_statements_reset()
is provided to pg_read_all_stats role in [1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=25fff40798fc4ac11a241bfd9ab0c45c085e2212#patch8.

The execute permissions grant to pg_read_all_stats concern is raised in [2]/messages/by-id/CAHGQGwE8Roydg-bVweK8GjpOipWh3rAAstMqoUfz7qBFrs8Tuw@mail.gmail.com
during the discussion of supporting different methods of reset the stats,
instead of resetting all.

Here I attached the patch that reverts the permission grant as per the
discussion
in [3]/messages/by-id/CAJrrPGdre9CqaHsbgtsc3LmJSRFg41fOZ+ZVpX9zu3D1fGqAeQ@mail.gmail.com.

[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=25fff40798fc4ac11a241bfd9ab0c45c085e2212#patch8
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=25fff40798fc4ac11a241bfd9ab0c45c085e2212#patch8
[2]: /messages/by-id/CAHGQGwE8Roydg-bVweK8GjpOipWh3rAAstMqoUfz7qBFrs8Tuw@mail.gmail.com
/messages/by-id/CAHGQGwE8Roydg-bVweK8GjpOipWh3rAAstMqoUfz7qBFrs8Tuw@mail.gmail.com
[3]: /messages/by-id/CAJrrPGdre9CqaHsbgtsc3LmJSRFg41fOZ+ZVpX9zu3D1fGqAeQ@mail.gmail.com
/messages/by-id/CAJrrPGdre9CqaHsbgtsc3LmJSRFg41fOZ+ZVpX9zu3D1fGqAeQ@mail.gmail.com

Regards,
Haribabu Kommi
Fujitsu Australia

Attachments:

0001-Revoke-pg_stat_statements_reset-permissions_v2.patchapplication/octet-stream; name=0001-Revoke-pg_stat_statements_reset-permissions_v2.patchDownload

From cb5b8904d6a7293dd08f3b10045de182efc9b5dd Mon Sep 17 00:00:00 2001
From: Hari Babu <kommi.haribabu@gmail.com>
Date: Mon, 9 Jul 2018 15:15:09 +1000
Subject: [PATCH] Revoke pg_stat_statements_reset() permissions

Commit 25fff40798 has granted the execute permissions of the
pg_stat_statements_reset() function to "pg_read_all_stats" role.
As this role is meant to read the stats, but not to reset.
So revoke the permissions on reset() function from "pg_read_all_stats".
---
 contrib/pg_stat_statements/Makefile                         | 3 ++-
 contrib/pg_stat_statements/pg_stat_statements--1.5--1.6.sql | 7 +++++++
 contrib/pg_stat_statements/pg_stat_statements.control       | 2 +-
 3 files changed, 10 insertions(+), 2 deletions(-)
 create mode 100644 contrib/pg_stat_statements/pg_stat_statements--1.5--1.6.sql

diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 39b368b70e..b1f9954bd6 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -4,7 +4,8 @@ MODULE_big = pg_stat_statements
 OBJS = pg_stat_statements.o $(WIN32RES)
 
 EXTENSION = pg_stat_statements
-DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.4--1.5.sql \
+DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.5--1.6.sql \
+	pg_stat_statements--1.4--1.5.sql \
 	pg_stat_statements--1.3--1.4.sql pg_stat_statements--1.2--1.3.sql \
 	pg_stat_statements--1.1--1.2.sql pg_stat_statements--1.0--1.1.sql \
 	pg_stat_statements--unpackaged--1.0.sql
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.5--1.6.sql b/contrib/pg_stat_statements/pg_stat_statements--1.5--1.6.sql
new file mode 100644
index 0000000000..1b7e081fe9
--- /dev/null
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.5--1.6.sql
@@ -0,0 +1,7 @@
+/* contrib/pg_stat_statements/pg_stat_statements--1.5--1.6.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION pg_stat_statements UPDATE TO '1.6'" to load this file. \quit
+
+-- Don't want this to be available to non-superusers.
+REVOKE EXECUTE ON FUNCTION pg_stat_statements_reset() FROM pg_read_all_stats;
diff --git a/contrib/pg_stat_statements/pg_stat_statements.control b/contrib/pg_stat_statements/pg_stat_statements.control
index 193fcdfafa..617038b4c0 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.control
+++ b/contrib/pg_stat_statements/pg_stat_statements.control
@@ -1,5 +1,5 @@
 # pg_stat_statements extension
 comment = 'track execution statistics of all SQL statements executed'
-default_version = '1.5'
+default_version = '1.6'
 module_pathname = '$libdir/pg_stat_statements'
 relocatable = true
-- 
2.16.1.windows.4

Michael Paquier

michael@paquier.xyz

over 7 years ago

In reply to: Haribabu Kommi (#1)

Re: Revoke execution permission of pg_stat_statements_reset() from pg_read_all_stats role

On Mon, Sep 24, 2018 at 11:08:14AM +1000, Haribabu Kommi wrote:

In commit 25fff40798 the execute permission of pg_stat_statements_reset()
is provided to pg_read_all_stats role in [1].

The execute permissions grant to pg_read_all_stats concern is raised in [2]
during the discussion of supporting different methods of reset the stats,
instead of resetting all.

Here I attached the patch that reverts the permission grant as per the
discussion
in [3].

This should be back-patched. Any opinions about bumping up this
extension version in back-branches like what has been done in 53b79ab4?
--
Michael

Tom Lane

tgl@sss.pgh.pa.us

over 7 years ago

In reply to: Michael Paquier (#2)

Re: Revoke execution permission of pg_stat_statements_reset() from pg_read_all_stats role

Michael Paquier <michael@paquier.xyz> writes:

This should be back-patched. Any opinions about bumping up this
extension version in back-branches like what has been done in 53b79ab4?

Yes, you need to bump the extension version to change anything in the
extension's script file.

For v10 and up, the method used in 53b79ab4 is overcomplicated: you only
need to add a delta script not a new base script. (If you had to
back-patch before v10, it might be best to add a new base script in all
the branches just to keep the patches consistent; but IIUC this issue only
arises in v10 and up.) I'd consider following, eg, 7f563c09f as a
prototype instead.

regards, tom lane

Michael Paquier

michael@paquier.xyz

over 7 years ago

In reply to: Tom Lane (#3)

Re: Revoke execution permission of pg_stat_statements_reset() from pg_read_all_stats role

On Mon, Sep 24, 2018 at 12:02:35PM -0400, Tom Lane wrote:

For v10 and up, the method used in 53b79ab4 is overcomplicated: you only
need to add a delta script not a new base script. (If you had to
back-patch before v10, it might be best to add a new base script in all
the branches just to keep the patches consistent; but IIUC this issue only
arises in v10 and up.) I'd consider following, eg, 7f563c09f as a
prototype instead.

Of course, thanks. Sorry for the incorrect reference pointing to a
commit of REL9_6_STABLE. As the patch only needs to be applied down to
v10, there is no need to do anything more complicated than what Hari has
proposed. So, committed after a bit of comment and format tweaks.
--
Michael

Haribabu Kommi

kommi.haribabu@gmail.com

over 7 years ago

In reply to: Michael Paquier (#4)

Re: Revoke execution permission of pg_stat_statements_reset() from pg_read_all_stats role

On Tue, Sep 25, 2018 at 10:58 AM Michael Paquier <michael@paquier.xyz>
wrote:

On Mon, Sep 24, 2018 at 12:02:35PM -0400, Tom Lane wrote:

For v10 and up, the method used in 53b79ab4 is overcomplicated: you only
need to add a delta script not a new base script. (If you had to
back-patch before v10, it might be best to add a new base script in all
the branches just to keep the patches consistent; but IIUC this issue

only

arises in v10 and up.) I'd consider following, eg, 7f563c09f as a
prototype instead.

Of course, thanks. Sorry for the incorrect reference pointing to a
commit of REL9_6_STABLE. As the patch only needs to be applied down to
v10, there is no need to do anything more complicated than what Hari has
proposed. So, committed after a bit of comment and format tweaks.

Thanks for the changes and commit.

Regards,
Haribabu Kommi
Fujitsu Australia