Horton

Thread

Commits

  1. Allow makeaclitem() to accept multiple privilege names.

  1. Allow makeaclitem() to accept multiple privileges

    Tharakan, Robins <tharar@amazon.com> — 2022-05-27T07:03:52Z

    Hi,

Presently, makeaclitem() allows only a single privilege in a single call.
This
patch allows it to additionally accept multiple comma-separated privileges.

The attached patch reuses the has_foo_privileges() infrastructure and
besides
a minor change to the function documentation, it also adds 3 regression
tests
that increase the function code-coverage to 100%.

Sample usage:

postgres=# SELECT makeaclitem('postgres'::regrole, 'usr1'::regrole,
  'SELECT, INSERT, UPDATE, ALTER SYSTEM', FALSE);
    makeaclitem
--------------------
 postgres=arwA/usr1
(1 row)

The need for this patch came up during a recent customer experience, where
'pg_init_privs.initprivs' had grantees pointing to non-existent roles. This
is
easy to reproduce [5] and given that this issue was blocking the
customer's planned upgrade, the temporary solution was to UPDATE the
initprivs column. From what I could see, there was a fix for similar
issues [1], although that didn't fix this specific issue [2] and thus
manually
modifying initprivs was required. For this manual update though, if the
proposed feature was available, it would have helped with the UPDATE SQLs.

To elaborate the customer issue, in most rows aclitems[]::TEXT was 2000+
characters long and spanned 30+ missing roles and multiple databases. In
trying
to automate the generation of UPDATE SQLs, I tried to use aclexplode() to
filter-out the missing grantee roles, however re-stitching the remaining
items
back into an aclitems[] array was non-trivial, since makeaclitem() doesn't
yet
accept multiple privileges in a single call. In particular, the
unnest() + string-search approach mentioned in this thread [4] didn't scale
with many missing roles where rolenames were alphanumeric.

See [6] for a contrived example where the updated makeaclitem() can be used
to
regenerate the initprivs column, weeding out privileges related to missing
grantees.

Lastly, while researching, I saw a thread [3] questioning whether
makeaclitem() is useful, and think that if it were to allow multiple
privileges,
it could have helped in the above situation, and thus I'd -1 on dropping the
function.

Reference:
1)
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=47088c599
cc6d6473c7b89ac029060525cf086d8

2)
https://www.postgresql.org/message-id/flat/29913.1573835973%40sss.pgh.pa.us#
90b0192c126ea61266e31dbb864c9b08

3)
https://www.postgresql.org/message-id/flat/48f9156d-3937-cf47-13ee-ac4e90c83
c43%40postgresfriends.org#7f5c830819bc104c222b440689d2028f

4)
https://www.postgresql.org/message-id/flat/1573808483712.96817%40Optiver.com

5) Reproduction to get pg_init_privs.initprivs to point to missing roles.

psql -c "CREATE DATABASE w" postgres
export PGDATABASE=w;
psql -c "CREATE USER usr1 PASSWORD 'usr1' SUPERUSER;"
psql -U usr1 -c "CREATE EXTENSION pg_stat_statements" -c "SELECT * FROM
pg_init_privs WHERE privtype = 'e'" -c "REASSIGN OWNED BY usr1 TO postgres;"
psql -c "DROP USER usr1" -c "SELECT * FROM pg_init_privs WHERE privtype =
'e';"
.
.
.
This would end up with something like this:

r=# select * from pg_init_privs where privtype = 'e';
 objoid | classoid | objsubid | privtype |
initprivs
--------+----------+----------+----------+----------------------------------
-------------------------------------
  16433 |     1255 |        0 | e        | {16425=X/16425}
  16441 |     1259 |        0 | e        |
{16425=arwdDxt/16425,=r/16425,t=r/postgres}
  16452 |     1259 |        0 | e        |
{16425=arwdDxt/16425,=r/16425,uw22341=arwdDxt/postgres,t=rw/postgres}
(3 rows)

6) This feature can then be used to generate the UPDATE SQLs to fix the
issue:

r=# BEGIN;
BEGIN
r=*# WITH a AS (SELECT pg_init_privs.* ,(aclexplode(initprivs)).* FROM
pg_init_privs WHERE privtype = 'e')
r-*# SELECT DISTINCT 'UPDATE pg_init_privs SET initprivs = ''{' || (
r(*#     WITH x AS (
r(*#         SELECT (aclexplode(initprivs)).*
r(*#         FROM pg_init_privs
r(*#         WHERE objoid = a.objoid AND classoid = a.classoid AND objsubid
= a.objsubid
r(*#         )
r(*#       ,y AS (
r(*#         SELECT makeaclitem(grantee, grantor, string_agg(privilege_type,
','), is_grantable) p
r(*#         FROM x
r(*#         WHERE grantee IN (SELECT oid FROM pg_roles)
r(*#         GROUP BY grantee,grantor,is_grantable
r(*#         )
r(*#     SELECT string_agg(p::TEXT, ',') FROM y
r(*#     ) || '}'' WHERE objoid=' || a.objoid || ' AND classoid=' ||
a.classoid || ' AND objsubid=' || a.objsubid || ';'
r-*# FROM a;
                                                               ?column?
----------------------------------------------------------------------------
----------------------------------------------------------
 UPDATE pg_init_privs SET initprivs = '{t=r/postgres}' WHERE objoid=16441
AND classoid=1259 AND objsubid=0;
 UPDATE pg_init_privs SET initprivs =
'{uw22341=arwdDxt/postgres,t=rw/postgres}' WHERE objoid=16452 AND
classoid=1259 AND objsubid=0;

(3 rows)

r=*# \gexec
UPDATE 1
UPDATE 1
r=*# select * from pg_init_privs where privtype = 'e';
 objoid | classoid | objsubid | privtype |                initprivs
--------+----------+----------+----------+----------------------------------
--------
  16433 |     1255 |        0 | e        | {16425=X/16425}
  16441 |     1259 |        0 | e        | {t=r/postgres}
  16452 |     1259 |        0 | e        |
{uw22341=arwdDxt/postgres,t=rw/postgres}
(3 rows)

(Similarly, it should be possible to generate DELETE SQLs for rows with
*all*
ACL objects pointing to missing roles - for e.g. row 1 above).

-
Robins Tharakan
Amazon Web Services

  2. Re: Allow makeaclitem() to accept multiple privileges

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-03T20:50:44Z

    "Tharakan, Robins" <tharar@amazon.com> writes:
> Presently, makeaclitem() allows only a single privilege in a single call.
> This
> patch allows it to additionally accept multiple comma-separated privileges.

Seems reasonable.  Pushed with minor cosmetic adjustments (mostly
docs changes).

			regards, tom lane